Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial Spec #9

Merged
merged 17 commits into from
May 13, 2024
Merged

Initial Spec #9

merged 17 commits into from
May 13, 2024

Conversation

guybedford
Copy link
Collaborator

@guybedford guybedford commented May 2, 2024

This implements some initial spec to work through the hooks model.

The spec text here is based to the source phase imports proposal specification, performing a new diff on top of that. More spec text than is needed is included while we work through the integration points.

This currently implements:

  • Converting the HostGetModuleSourceName hook into a HostGetModuleRecordFromSource hook.
  • Support for dynamic import of a module source, where any object is passed to the host hook, but any object which is not a module source should return unused from this host hook to still go through ToString. In addition a new error is thrown if the realms do not match, and this seems like it sets a good precedent for future concepts of compartments matching. This came together relatively simply in the model.
  • Implementation of GetModuleSource for a Source Text Module Record, which also includes now an explicit [[ModuleSource]] slot to cache this on cyclic module records, now that we actually have a valid implementation.
  • Support for a new ModuleSource builtin object extending abstract module source
  • Definition for AbstractModuleSource.prototype.imports, as well as getters for get AbstractModuleSource.prototype.hasDynamicImport, hasImportMeta and hasTopLevelAwait

One slightly weird thing about the hooks model is that HostGetModuleRecordFromSource applies to BOTH the local ModuleSource and other types of module source, so there's wording saying "if it's a ModuleSource, it must conform to the abstract operation for ModuleSource that uses the [[SourceTextModuleRecord]] internal slot". I think that gets us the right flexibility but feedback ensuring clarify here would be very welcome.

Hopefully this forms a good initial skeleton of the hook model further feedback and discussion.

Follow-up work can then be investigated further:

  • Universal keying of sources between realms by abstracting out the host defined and request property data.
  • Separation of the module source record from cyclic module record.

Rendered Spec: https://guybedford.github.io/proposal-module-instance-imports/build/

@guybedford guybedford changed the title WIP: Draft Spec Draft Spec May 2, 2024
spec.emu Outdated Show resolved Hide resolved
@guybedford guybedford marked this pull request as ready for review May 3, 2024 01:03
@guybedford guybedford changed the title Draft Spec Initial Spec May 3, 2024
@guybedford
Copy link
Collaborator Author

@nicolo-ribaudo I think this spec text is finally ready, I'd be really grateful if you could review when you have a moment.

spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated
1. Perform ! CreateDataPropertyOrThrow(_import_, *"phase"*, _maybePhaseString_).
1. Append _import_ to _imports_.
1. Let _importsArray_ be CreateArrayFromList(_imports_).
1. Perform ! CreateDataPropertyOrThrow(_metadata_, *"imports"*, _importsArray_).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why there is no "exports" list? it is useful if the module has not been initialized yet

Copy link
Collaborator Author

@guybedford guybedford May 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difficulty in specifying exports is that we only have this information available for modules at runtime usually. So the hooks model doesn't deal with it being processed in any way before.

We could support exports() for JS modules only (eg putting it on ModuleSource.prototype instead of on AbstractModuleSource.prototype).

Alternatively, we could treat it as a special method to be implemented by any concrete cyclic module record implementation.

README.md Outdated Show resolved Hide resolved
1. <ins>Let _module_ be Completion(HostGetModuleSourceRecord(_specifier_)).</ins>
1. <ins>IfAbruptRejectPromise(_module_, _promiseCapability_).</ins>
1. <ins>If _module_ is not ~unused~, then</ins>
1. <ins>If _module_.[[Realm]] is not equal to current Realm Record, throw a *TypeError* exception.</ins>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why we cannot import from another realm?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in the meeting this is a placeholder for now, to be further refined in future as a Stage 2.7 question rather than a Stage 2 question, due to the difficulty in specifying this requiring a new spec refactoring.

The spec refactoring in question would be to separate compiled module records from instance module records like Source Text Module, so that we can have a realm-agnostic backing record. We may want to do this refactoring after import attributes merges in July, but before this proposal reaches Stage 2.7.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some editorial comments, but it looks overall good.

spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
@guybedford guybedford merged commit 65d30b8 into main May 13, 2024
1 check passed
@guybedford guybedford deleted the initial-spec branch May 13, 2024 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants