Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cyclic dependencies in es6 modules #871

Closed
jonathanolson opened this issue Feb 6, 2020 · 13 comments
Closed

Cyclic dependencies in es6 modules #871

jonathanolson opened this issue Feb 6, 2020 · 13 comments

Comments

@jonathanolson
Copy link
Contributor

(JO, [SR?] 2-4 hours) [sprintable, JO investigated] Cyclic Dependencies: ES6 modules support cyclic dependencies. So places like scenery and dot, where we had to "trick" the runtime with an assignment-less require statement and then loading things from the namespace will no longer be necessary.
What to do with this pattern and cycles in general? window.require = function(){}; // TODO: #820 this supports like require( 'DOT/Utils' ); in Vector2
JO: Cyclic dependencies in our classic style look fine. See #820 (comment) for details, use empty imports (import ‘../repo/js/Blah.js’) if we just need to ensure code runs (our migrate will need to change to include this).
JO: Commented-out require statements could be added back in, and use the cyclic form

I'll investigate commented-out require statements, we can potentially just uncomment those and test.

After the fact, I'll also want to do some cleanup in those cases

@jonathanolson
Copy link
Contributor Author

Handled, we're now uncommenting those require statements and it's working nicely.

Keeping open to handle cleanup after full migrate, but dropping priority.

@samreid
Copy link
Member

samreid commented Feb 21, 2020

Balloons and static electricity exhibits a problem where scenery.Image does not exist when Node.rasterized is called. @jonathanolson how do you recommend to proceed?

@samreid
Copy link
Member

samreid commented Feb 21, 2020

From slack:

Jonathan Olson 5:55 PM
Just have an empty import in Node?

Sam Reid 6:00 PM
I tried but that creates a cycle and fails to load. Image cannot be defined until Node definition completes

Jonathan Olson 6:01 PM
Then put an empty import where we use rasterize?

Sam Reid 6:02 PM
At each of the top-level call sites?

@jonathanolson
Copy link
Contributor Author

Created phetsims/scenery#1038 for handling that case. I presume we aren't running into any other issues?

@samreid
Copy link
Member

samreid commented Feb 21, 2020

As far as I can tell, this only triggered in Balloons and Static Electricity, and the workaround I committed seems to be working OK.

@chrisklus
Copy link
Contributor

@jonathanolson is there anything to do here? @samreid thinks this might be ready to close.

@chrisklus chrisklus assigned jonathanolson and unassigned samreid Feb 26, 2020
@jonathanolson
Copy link
Contributor Author

Keeping it open for #871 (comment), since things like Vector2/Vector3 will want to not JUST import each other, but USE those imports (which I can't do easily in master before migration).

jonathanolson added a commit to phetsims/scenery that referenced this issue Apr 13, 2020
jonathanolson added a commit to phetsims/scenery that referenced this issue Apr 13, 2020
jonathanolson added a commit to phetsims/scenery that referenced this issue Apr 13, 2020
@jonathanolson
Copy link
Contributor Author

Handled most circular dependency issues. Things like Block/Drawable seem like an issue (where interestingly if we try to import Block from Drawable, it will fail out and I can't identify where that import would cause issues.

@samreid thoughts to continue?

@jonathanolson
Copy link
Contributor Author

The above commit seems to resolve scenery builds, but that's really concerning. @samreid have time to discuss this sometime?

@samreid
Copy link
Member

samreid commented Apr 15, 2020

Since the commit includes sorting, it is difficult to tell what part is concerning. Can you please clarify?

@samreid samreid assigned jonathanolson and unassigned samreid Apr 15, 2020
@jonathanolson
Copy link
Contributor Author

That removing the import for ParallelDOM in the scenery main.js now allows it to run correctly after building.

And the fact that there were a lot of reasons I couldn't add back in all the imports we wanted (Block/Drawable being most easy to reproduce).

@jbphet
Copy link
Contributor

jbphet commented Jun 4, 2020

We discussed this in the 6/4/202 dev meeting and the memorable quote was "[T]his is low priority until it becomes a problem." So, we are going to mark it as deferred and assign it to @ariel-phet. He'll set a reminder to check it in six months and, if it's not a problem by then, we will close it.

@ariel-phet
Copy link
Contributor

Calendar reminder set

@ariel-phet ariel-phet removed their assignment Jun 9, 2020
@zepumph zepumph closed this as completed Dec 3, 2020
zepumph pushed a commit to phetsims/scenery that referenced this issue Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants