-
Notifications
You must be signed in to change notification settings - Fork 46
Circular reference handling #35
Comments
How about option 4: Throw an exception when detecting a cycle containing a module with TLA? I can't think of any good use case where dependencies are expected to race with each other. Someone will eventually get bitten hard in either case. Disallowing it completely might be the best idea. If use cases come up, we can still add support for it later without breaking any working code. |
I don't like option 3. |
Some food for thought:
|
I was expecting 3. , but if 4 is acceptable, I'd be happy to go with that option as well. I don't understand the intuition for 1 or 2. Any other thoughts, on either this question or whether we could throw in other kinds of circularity cases? cc @GeorgNeis @MylesBorins @domenic |
It would be surprising if the addition of a top-level-await somewhere changed The current (synchronous) specification is carefully designed to support cycles, and I don't think that the addition of TLA should compromise on that design. None of the 4 options presented here seem to work. Would the following work?
It's difficult for me to tell whether #33 moves in this direction or not. |
@zenparsing in this model is |
@guybedford Not exactly either; it's a promise for when the module transitions from "evaluating" to "evaluated". In the same way that all modules in an SCC transition to "evaluated" together, all modules in an SCC would have their |
@zenparsing right, but the question then does one top-level evaluation job "own" that evaluation promise transition? Or is it just the first top-level evaluation job that does that transition? |
I'm confused about the "own" terminology, so I'm not sure how to answer. I think the best way to talk about this is by using the Tarjan's algorithm/SCC terminology. The evaluation promise would be resolved by that algorithm (when the module is transitioned from "evaluating" to "evaluated"), not by any job. |
So right now the spec will transition the components together, but the complication with top-level wait is that this algorithm is now being changed from "sync" to "async", where new top-level jobs can be started during the async resolution. So we need to be handling the fact that there are potentially multiple runs of this algorithm happening simultaneously, where they may have overlapping modules. |
Yes, part of the design validation should be proving that the "asyncified" version of the SCC algorithm still works in those overlapping cases. |
I'm not sure if this algorithm is as "async-ified" as it may sound. In #33, I don't believe any module is left in the "evaluating" state when we get to the next turn. We are just left with some Promises for "evaluated" modules which might not be resolved yet (and might never). What do you see as remaining to be proven? |
If the graph walk is not "asyncified" then I don't understand how this is supposed to guarantee that dependencies are fully evaluated prior to evaluation. Perhaps I just don't understand the proposal and need to see the algorithm in full (instead of a PR). Also, I think we should be more conservative and try variant A first. |
My understanding of the "B" algorithm is that the SCC component search and cycle breaking happens synchronously, while actually launching the module body will block on the dependencies' promises resolving. Seems like we need to improve documentation here to explain the algorithms more clearly. I am looking into this. Also happy to discuss at a future TC39 meeting. What do you mean "try first"? I think we have to make a decision on one or the other (or decide against this proposal). Anyway, I don't think A vs B affects the async-ness of the module evaluation graph walk. |
@littledan Are you championing this proposal now? |
No, but it's blocking my work on WebAssembly modules, so I am pitching in to help. |
Oh, cool. You may have mentioned this elsewhere, but why does TLA block wasm modules? I wasn't aware that there was any awaiting going in in wasm "instantiation". |
In the proposal in WebAssembly/esm-integration#13 (which I have to do more work on--to clarify, @linclark handed over championing the Wasm/ESM integration proposal to me) , there is awaiting going on in WebAssembly module evaluation, not instantiation, to instantiate the WebAssembly module. In effect, the proposed semantics are to do a top-level await on the Wasm instantiation, and then instantiate the JS bindings exported based on that. See more discussion in WebAssembly/esm-integration#20 . WebAssembly modules wouldn't block on actually exposing |
Is this because wasm has to "snapshot" the imported bindings, and so cycles don't work correctly (because live bindings don't propagate) without deferring evaluation to a later job? |
(Still trying to gather the context...) Isn't "instantiation" in wasm always synchronous (as opposed to "compile", which can be done in a non-blocking manner)? |
That's right, the instantiation itself is synchronous, since it needs to interact with JS; what I am talking about making asynchronous is roughly instantiateStreaming (without the streaming part), which combines compilation and instantiation. This is necessary since some platforms end up needing the imports to do compilation. |
Interesting... But don't you need to compile ahead of time in order to get the list of exports for the ES instantiation pass? So that importing ES modules know the list of export names? |
Yep, that's right, but on some engines this might end up as more of a parse than a compilation. |
The current specification text resolves static cycles by breaking the cycle just like for synchronous modules, and not awaiting on the cycle-completing edge. The Promise for each module in the cycle is the Promise of the entry-point. If you see any problems with these semantics, please open an issue to discuss them. |
In a cycle like the following:
a.js
b.js
If I import
a.js
,b.js
should execute first.On the other hand, if I import
b.js
,a.js
should execute first.But what happens if I do both one after the other -
import('a.js'), import('b.js')
.The text was updated successfully, but these errors were encountered: