Module loading #1197
Open
Module loading #1197
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR introduces two major changes to the ScriptManager. 1 - Simplification. Rather than having a `Script`, `PendingScript`, `AsyncModule` and `SyncModule`, there is only a `Script`, with an added `mode` union. All of the previous objects had the same behavior (collect the response in a buffer), up to the point of execution, which is where the mode comes in. 2 - Correctness Whether or not the previous version was "incorrect", it was difficult to use correctly. Specifically, the previous version would execute async scripts and async modules as soon as they're done. That seems allowed, but it caused issues with module loading in Context.js. Specifically, between compiling and instantiating a module, or between instantiation and evaluation, an async script or module could be evaluated. It isn't clear whether v8 allows that, but if it does, it introduces a lot of new potential states (specifically, unexpected changes to the v8.Module's status) that we have to handle. This version only evaluate scripts in the `evaluate`, which doesn't allow recursive calls (so a waitForImport, which continues to pump the HTTP loop, can never result in `evaluate` being called again). This undoes the change made in #1158 because I do not think it's possible to have multiple waiters waiting for the same (or even different) modules. The linked issue points to a crash in https://www.nytimes.com which doesn't crash with this version.
This does two changes to module loading. First, for normal imports, it only
instantiates and evaluates the top-level module. This ensures that circular
dependencies can be resolved. This bug was introduced when I tried to
deduplicate code between dynamic and normal modules - but it turns out that
non-top-level normal modules do have a simpler flow (they just need to be
compiled, and we let v8 deal with the rest).
The other change is to handle more edge cases. Code like this should now be ok:
```
<script type=module>
var a = await import('a.js');
</script>
<script type=module>
import a from a.js
</script>
```
Previously, the dynamic import of a.js (first block) could interact badly with
the normal import of a.js in the 2nd block.
This change is built on top of #1191
which also helps reduce the number of cases by ensure that a script isn't
evaluated while we're trying to evaluate a script.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This does two changes to module loading. First, for normal imports, it only
instantiates and evaluates the top-level module. This ensures that circular
dependencies can be resolved. This bug was introduced when I tried to
deduplicate code between dynamic and normal modules - but it turns out that
non-top-level normal modules do have a simpler flow (they just need to be
compiled, and we let v8 deal with the rest).
The other change is to handle more edge cases. Code like this should now be ok:
Previously, the dynamic import of a.js (first block) could interact badly with
the normal import of a.js in the 2nd block.
This change is built on top of #1191
which also helps reduce the number of cases by ensuring that a script isn't
evaluated while we're trying to evaluate a script.