-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Editorial: dedupe and isolate modules traversal logic #3635
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
base: main
Are you sure you want to change the base?
Conversation
a4fc0b9
to
e2975da
Compare
c87144b
to
1831e10
Compare
1831e10
to
acc3d85
Compare
Marking as draft as:
|
64f28a0
to
aa7eab7
Compare
The rendered spec for this PR is available at https://tc39.es/ecma262/pr/3635. |
aa7eab7
to
f974a14
Compare
f945022
to
21a4aab
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.
Also, this PR un-defines the following ids:
- sec-InnerModuleLinking
- sec-innermoduleevaluation
- sec-innermoduleinstantiation
which should maybe be retained in oldids
attributes.
Any preference on whether the oldid for |
…loop Rather than doing it in the DFS recursion loop. This changes the order in which modules are pushed in [[PendingAsyncDependencies]]: now they automatically are sorted by their [[AsyncEvaluationOrder]]. It does not actually matter though, because GatherAvailableAncestors will merge various [[PendingAsyncDependencies]] lists in a non-order-preserving way so AsyncModuleExecutionFulfilled still needs to explicitly re-sort the resulting list.
This commit deduplicates the logic that is currently both in InnerModuleLinking and InnerModuleEvaluation, clearly splitting what logic is relative to traversing the graph and what is relative to actually linking/evaluating modules.
892a45f
to
309df93
Compare
Me? Maybe a slight pref for BTW, just noticed that the PR leaves lots of dangling references to |
This PR separates the logic to perform a depth-first traversal on module graph out of InnerModuleLinking/InnerModuleEvaluation. It has two advantages:
Whether this is an improvement or not is subjective (it adds a layer of indirection), so I'm
curious to read the editors opinion about it.
From an editorial/formatting point of view, ModuleGraphDFS is an AO that takes a bunch of callbacks. I've declared them inline in the
Link()
/Evaluate()
concrete methods as abstract closures, even though they don't actually close over anything. I think having them inline rather than as separate AOs makes the reading flow simpler. Ideally, my preferred approach would be something that lets us writeI suggest reading this commit-by-commit. The first two commits just move logic around so that what would go in the same AC of the third commit is already all in the same place.
This PR preserves the bugs that would be fixed by #3613 and #3583, but by implementing this PR + those two in engine262 all the tests I have still pass.
The
import defer
proposal would add a new callback toModuleGraphDFS
,_getModuleDescendants_
, that for linking just returns the RequestedModules while for Evaluation it uses import defer's step 11 of https://tc39.es/proposal-defer-import-eval/#sec-innermoduleevaluation (with GatherAsynchronousTransitiveDependencies).