-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Editorial: Explicitly track async evaluation order of pending modules #3353
Conversation
d600852
to
f96a304
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thanks for making this clearer.
As you may have seen in #3356, there's a bug in this machinery. The fix is a one liner so we should fix that bug, and you'll need to rebase this on top of the fix.
4cdd981
to
b1c782e
Compare
PR updated, sorry I forgot about this. For #3353 (comment), I pushed it in a second commit to make it easier to review where I'm using |
d5527c1
to
0789693
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with my comment. This is so much nicer, thanks.
Don't forget to update https://tc39.es/proposal-defer-import-eval/#sec-example-cyclic-module-record-graphs-deferred-imports after this lands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise. I'm just trusting that the numbers in the "Example Cyclic Module Record Graphs" table are correct.
Ready except it needs the Edit: I went ahead and did this myself. |
…tc39#3353) The execution order of modules that were waiting on a given async module was based on the order in which their [[AsyncEvaluation]] field was set to *true*. This PR replaces the [[AsyncEvaluation]] boolean with an actual number that keeps track of ordering in each agent. It also adds a note on when it's safe to reset this order to 0, since: - implementations might want to prevent this number from overflowing, and V8 currently has a bug that resets the number to 0 too early - figuring out when it's safe to reset the number might not be trivial, given the complexity of the module algorithms.
ca9388f
to
030dcd6
Compare
The execution order of modules that were waiting on a given async module was based on the order in which their [[AsyncEvaluation]] field was set to true.
This PR replaces the [[AsyncEvaluation]] boolean with an actual number that keeps track of ordering in each agent. It also adds a note on when it's safe to reset this order to 0, since:
You can verify the numbers in the updated tables for the example using https://nicolo-ribaudo.github.io/es-module-evaluation/.
Fixes #3289