-
Notifications
You must be signed in to change notification settings - Fork 284
Commit 70e02dc
committed
Refactor the ScriptManager
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.1 parent 8a867bc commit 70e02dcCopy full SHA for 70e02dc
File tree
Expand file treeCollapse file tree
2 files changed
+305
-487
lines changedOpen diff view settings
Filter options
- src/browser
- js
Expand file treeCollapse file tree
2 files changed
+305
-487
lines changedOpen diff view settings
0 commit comments