Layering: Add HostLoadImportedModule hook#2905
Conversation
bac620f to
36635e0
Compare
e73116d to
407474b
Compare
407474b to
891949f
Compare
|
The HTML side of this refactor is starting to get reviewed, I would love to receive a review for this PR too! |
guybedford
left a comment
There was a problem hiding this comment.
It's possible that one day HTML will align requests of static and dynamic imports, but today is not that day yet.
Would be interested to hear more!
ResolveExport is described as returning null or something, but it can also return a throw completion (if it's called on a module that re-exports from a dependency that hasn't been already loaded - NOTE: this can only happen if a host calls ResolveExport() even if LoadRequestedModules() doesn't succeed, and they don't have any good reason to do so):
I don't think there is any real-world expectation that has ever relied upon ResolveExport throwing, so my preference would be turning this into an internal assertion and enforcing the behaviour that it is only ever called on loaded modules. Is that possible to do?
| <p>An implementation of HostLoadImportedModule must conform to the following requirements:</p> | ||
| <ul> | ||
| <li> | ||
| The host environment must perform FinishLoadingImportedModule(_referrer_, _specifier_, _payload_, _result_), where _result_ is either a normal completion containing the loaded Module Record or a throw completion, either synchronously or asynchronously. |
There was a problem hiding this comment.
In the case where a module has already been loaded, is it up to the host whether it resolves synchronously or asynchronously? I wonder if we should mandate already-loaded modules return synchronously to avoid unnecessary divergence?
There was a problem hiding this comment.
Until last week the HTML spec was ambiguous and allowed loading of already-loaded modules to be asynchronous. I updated that spec to make it synchronous, but without first discussing it with implementers, and browsers don't currently agree.
I'm working on it in web-platform-tests/wpt#35949, but I prefer to keep it out of this PR (since the consensus was based on the fact that this was not observably normative in common hosts).
There was a problem hiding this comment.
Glad to hear you're working on it!
| </td> | ||
| <td> | ||
| This field is only used if [[Action]] is ~graph-loading~. It is true if the loading process has not finished yet, neither successfully nor with an error. | ||
| </td> |
There was a problem hiding this comment.
Do we really need a separate [[IsLoading]] state if it is equivalent to checking [[PendingModules]] > 0?
There was a problem hiding this comment.
It's not equivalent if the host calls FinishLoadingImportedModule with a throw completion: there might still be pending modules, but we stop the loading process.
3cc0cce to
e5360da
Compare
|
Oh, I forgot to say: This PR undefines the ids:
which should maybe become oldids. |
eac7558 to
389bf4f
Compare
|
Thanks for the review @jmdyck! I added |
|
@tc39/ecma262-editors is there anything I could do to help moving this PR forward? There are a bunch of proposals that needs to be updated on top of this, so having it reviewed&merged would be nice. There are some editorial questions I have (in the issue description), and you can probably give your opinion on them just reading the new spec text without first doing a deep review, so that I can start resolving them :) |
|
@nicolo-ribaudo We discussed some of your questions on the editor call, our preferences are:
|
|
@nicolo-ribaudo Can you try rebasing on |
6704c5e to
f2ea1ad
Compare
|
Still failing, maybe because Tobie left google (or at least, it's not linked anymore in his GitHub profile)? |
|
@nicolo-ribaudo ill take a look at it |
|
It's very strange; the passing tests found 146 authors, in the last day or two it's found 147 including tobie. I'll fix it by adding them to the Emeriti team, since they were employed at Google at the time. |
f2ea1ad to
0413272
Compare
This commit also removed the HostResolveImportedModule and HostImportModuleDynamically hooks * Layering: Add HostLoadImportedModule hook * fix(JWK review): some wrong variable names * html review: FinishLoadImportedModule -> FinishLoadingImportedModule * guybedford review * jmdyck review * Add missing _state_.[[IsLoading]] check * syg review + add missing "Return ~unused~" steps * Require LoadRequestedModules before ResolveExport and GetExportedNames
0413272 to
18d41aa
Compare
This PR also removes the HostResolveImportedModule and HostImportModuleDynamically hooks.
A few comments/questions:
.destinationflag it sets on theRequestobject it creates to fetch modules). It's possible that one day HTML will align requests of static and dynamic imports, but today is not that day yet.ModuleLoadStaterecord, which is overloaded for two usages that are distinguished using an [[Action]] enum field. Is it ok, or should I split it into two different record types (GraphLoadingStateandDynamicImportState)? (related: Layering: Add HostLoadImportedModule hook #2905 (comment))The HTML PR is whatwg/html#8253. Today's consensus was based on the fact that this refactor is purely editorial for the ECMA-262+(insert known host here) pair, so I would like to get a full review on the HTML side to check if we need any tweaks before moving on with this PR. EDIT: Got a 👍 from the HTML side.