Layering: Add HostLoadImportedModule hook

[nicolo-ribaudo]

This PR also removes the HostResolveImportedModule and HostImportModuleDynamically hooks.

A few comments/questions:

1. ContinueDynamicImport is kinda messy, with all the closures and PerformPromiseThen. Is it ok to extend AsyncBlockStart like in https://tc39.es/proposal-array-from-async/#sec-asyncblockstart so that I can use the Await macro?
2. 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):
   • Is it ok?
   • Should we make it return null?
   • Should we require that it's called only after having loaded the dependencies?
3. I named the new Module Records method LoadRequestedModules() to match the name of the [[RequestedModules]] slot, but maybe LoadImportedModules() or LoadDependencies() are better?
4. I find the hostDefined parameter of LoadRequestedModules() to be incredibly ugly, but it's needed by HTML to pass some context only to modules imported in "the current loading process" and not in "future loading processes" (the .destination flag it sets on the Request object 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.

The HTML PR is https://github.com/whatwg/html/pull/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 :+1: from the HTML side.

[nicolo-ribaudo]

The HTML side of this refactor is starting to get reviewed, I would love to receive a review for this PR too!

[jmdyck]

Oh, I forgot to say: This PR undefines the ids:

• sec-hostresolveimportedmodule
• sec-hostimportmoduledynamically
• sec-finishdynamicimport

which should maybe become oldids.

[nicolo-ribaudo]

Thanks for the review @jmdyck! I added sec-hostresolveimportedmodule/sec-hostimportmoduledynamically as oldids of HostLoadImportedModule, and sec-finishdynamicimport as oldids fo FinishLoadingImportedModule. However, I'm not sure if it's correct because those aren't just renames but completely new AOs.

[nicolo-ribaudo]

@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 :)

[syg]

@nicolo-ribaudo We discussed some of your questions on the editor call, our preferences are:

1. We have plans eventually to basically have asynchronous Abstract Closures. For now, since you already wrote the manual promise-handler version, just keep it as is, we'll simplify this when we introduce the ability to have async ACs.

2. We don't quite grok the scenario you're describing. Are you saying the only reason this method would throw today is if a host calls it in a weird way that no host actually does? If so, please help us confirm if that's the case, and once confirmed, we should add assertions and make the method infallible.

3. Name sounds good.

4. More of a comment than a question? Let's keep the host-defined.

5. Please name and split them into two separate Record types. Get rid of [[Action]] and just query for if the record is one of the specific record types.

[nicolo-ribaudo]

Thank you!

We don't quite grok the scenario you're describing. Are you saying the only reason this method would throw today is if a host calls it in a weird way that no host actually does? If so, please help us confirm if that's the case, and once confirmed, we should add assertions and make the method infallible

Today ResolveExport (ResolveExport in the current spec) can throw if one of its HostResolveImportedModule calls throw (steps 5.a.i and 8.a). In practice (i.e. in all the environments that have a pre-load phase, such as Node.js and the web) this means that it throws if the dependencies of the module have not been loaded yet. In theory some hosts could support loading the dependencies synchronously in HostResolveImportedModule and not throw even if they have been pre-loaded. At least one engine already always makes HostResolveImportedModule throw for not-yet-loaded modules, without giving embedders a choice (JSC).

With this PR (ResolveExpot in this PR) it will throw if the dependencies have not been pre-loaded, because now the pre-loading is in ecma262 and thus hosts do not have the choice anymore.

While in the current spec ResolveExport could only throw when "forwarding" host errors, now it doesn't call host hooks anymore and it has hard-coded throw a new TypeError steps: it has two failure modes, both 100% within ECMA-262, the other one being return null (for example, step 2.a.ii).

We could keep this behavior, but having a single failure mode looks cleaner:

1. we could always return null, rather than throwing some times and returning null other times
2. we could always throw
3. we could assert that hosts only call ResolveExport after a successful LoadRequestedModules call, keep the current return nulls, and delete the throw a new TypeErrors, since all the dependencies have already been loaded.

[nicolo-ribaudo]

I added a commit to do (3), which probably is the most sensible option. I can revert it if you prefer (1) or (2).

[nicolo-ribaudo]

@jmdyck Done, thanks!

[nicolo-ribaudo]

Regarding the IPR failure: I originally signed the IPR form with my personal email, but I have committed with my Igalia email. I'll fix it.

EDIT: It looks like the failure is not related? It says "Missing 1 authors: tobie"

[michaelficarra]

@nicolo-ribaudo Can you try rebasing on main?

[nicolo-ribaudo]

Still failing, maybe because Tobie left google (or at least, it's not linked anymore in his GitHub profile)?

[ljharb]

@nicolo-ribaudo ill take a look at it

[ljharb]

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.

[nicolo-ribaudo]

Thanks @ljharb :)

@jmdyck done!