tc39 / proposal-source-phase-imports

Proposal to enable importing modules at the source phase
https://tc39.es/proposal-source-phase-imports/
MIT License
132 stars 8 forks source link

Rely on the new modules loading logic instead of a custom hook #25

Closed nicolo-ribaudo closed 2 years ago

nicolo-ribaudo commented 2 years ago

Hi!

This PR includes the spec for import reflection aligned to what I believe is the shared vision we built during the last few months.

PREVIEW HERE

Also, this PR is written on top of https://github.com/tc39/ecma262/pull/2905. The ecma262-biblio.json file you see is to ensure proper linking of all the new AOs to that PR, until it's merged.

cc @littledan, @kriskowal and @caridy, who work on other pieces of this modules improvements effort.

ljharb commented 2 years ago

Does your OP encapsulate the entirety of the proposal? Meaning, there's no other semantics than what you've described?

nicolo-ribaudo commented 2 years ago

Yes. Or at least, it (the OP description, and the spec text in this PR) encapsulates the entirety of one of the possible versions of this proposal: the one which leaves less customizations to the host.

littledan commented 2 years ago

An essential aspect of this proposal will be the Wasm+HTML integration specification, where the .source of a Wasm module is the WebAssembly.Module. I suspect that we could do this without requiring the other compartments proposals to complete, just by saying it is undefined for JS modules for now, and returns that particular object for WebAssembly.

nicolo-ribaudo commented 2 years ago

Right now this PR makes it null for JS modules, and mentions WebAssembly.Module in the note of https://nicolo-ribaudo.github.io/proposal-import-reflection/#sec-module.prototype.source.

nicolo-ribaudo commented 2 years ago

Thanks for the review @guybedford! I reverted the normative changes and now I believe that this PR is purely editorial.

I ignored one of your suggestions: instead of tracking the reflection in an _importReflection_: null | ~module~ variable, I used a boolean. This is because an enum doesn't fit well with the concepts from the hooks refactoring and the other proposals: either you want to execute a module or you want to have it not executed (well, other than passing the evaluator attributes to the host).

nicolo-ribaudo commented 2 years ago

With these changes, the proposal works without almost any changes to host specs :)

We just need to rename [[WebAssemblyModule]] to [[ModuleSourceObject]] in https://webassembly.github.io/esm-integration/js-api/index.html#esm-integration.

nicolo-ribaudo commented 2 years ago

@guybedford I just realized that how we store the two type of imports and how we iterate over them is observable, because both modules evaluation order and modules loading order is observable (loading order is currently only observable by SharedWorkers and Node.js loaders API, but with compartments it would also be observable within ECMA-262).

Consider this example:

import module A from "A";
import module B from "B";
import "C";
import "A";
import "D";

With the spec text that I wrote, the loading order is CADB and the execution order is CAD.

I think that the execution order is fine, but the loading order maybe should be ABCD? Wdyt?

caridy commented 2 years ago

With the spec text that I wrote, the loading order is CADB and the execution order is CAD.

I think that the execution order is fine, but the loading order maybe should be ABCD? Wdyt?

I think it should be the one that matches the developer's intuitions, which in this case is ABCD, by just looking at the code.

guybedford commented 2 years ago

It doesn't make a huge difference to users since loads are parallel, but at least from a host consistency perspective, iterating the operations on the imports in pre-order continues to make sense. I think my suggested changes would do that as well?

nicolo-ribaudo commented 2 years ago

I updated the PR to load dependencies in the correct order.

@guybedford and I discussed about adding a note to discourage hosts from pre-loading dependencies in the HostLoadImportedModule hook, since it's also called for import reflections that don't need their dependencies to be loaded. Does this looks good, added as a fourth bullet point to https://ci.tc39.es/preview/tc39/ecma262/sha/24196c3212c3c4ac421e0cf9b002e58fb1445f6d/#sec-HostLoadImportedModule?

<li>
  <p>The host environment should not attempt loading the dependencies of the loaded Module Record.</p>
  <emu-note>
    When using `import module x from "x"`, the dependencies of `"x"` are not immediately needed and may never be needed. If the dependencies of an imported module need to be loaded, it will be done through further calls to HostLoadImportedModule.
  </emu-note>
</li>