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

Normative: add import.meta #1892

Merged
merged 1 commit into from Apr 2, 2020
Merged

Normative: add import.meta #1892

merged 1 commit into from Apr 2, 2020

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Mar 7, 2020

This is the spec text for the stage 3 proposal import.meta.

Tests are available in both test262 (tc39/test262#1888) and wpt (web-platform-tests/wpt#7888). Implementation status is very good (JavaScriptCore, QuickJS, SpiderMonkey, V8, Moddable XS, engine262, Babel, Rollup, probably more), and it is shipping in many browsers (87% on https://caniuse.com/#search=import.meta).

@devsnek devsnek force-pushed the import-meta branch 2 times, most recently from 5d844e2 to 062e652 Compare March 7, 2020 10:58
@ljharb ljharb added has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. labels Mar 7, 2020
spec.html Outdated Show resolved Hide resolved
spec.html Outdated
1. Set _module_.[[ImportMeta]] to _importMeta_.
1. Return _importMeta_.
1. Else,
1. Assert: Type(_importMeta_) is Object.
Copy link
Member

Choose a reason for hiding this comment

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

Should it be asserted that it is an ordinary object? iow, a proxy wouldn’t be valid here.

@michaelficarra
Copy link
Member

I don't love that we're adding two hooks, where one is strictly more powerful than the other. What's the point of the restrictions on the return values of HostGetImportMetaProperties if HostFinalizeImportMeta just gets to do whatever it wants? I'd much prefer that we just create an empty object, pass it to HostFinalizeImportMeta, and let that do what it wants.

On a separate note, I don't think the module record alone as a parameter to one/both of these host hooks is sufficient. I imagine a host may want to make the value of import.meta dependent on how the module was imported (declarative/dynamic), which module(s) imported the module, or additional meta-info as in the module attributes proposal.

@devsnek
Copy link
Member Author

devsnek commented Mar 12, 2020

@michaelficarra

I don't love that we're adding two hooks [...] I'd much prefer that we just create an empty object, pass it to HostFinalizeImportMeta, and let that do what it wants.

I'd be fine with that too. One thing to note is that the HTML spec uses HostGetImportMetaProperties.

dependent on how the module was imported (declarative/dynamic)

Those use identical spec machinery, I would argue that should never be visible information (and I'm not sure how it would be exposed anyway, since both can happen to the same module)

which module(s) imported the module

I'm not entirely sure what that entails but a host has access to its own module graph.

additional meta-info as in the module attributes proposal.

Modules have a [[HostDefined]] slot for whatever random stuff the host wants to throw in. I'm not sure how we would plan ahead for any module attributes stuff because that's still a stage 1 proposal.

@michaelficarra
Copy link
Member

Those use identical spec machinery

Do they? I'm not great with the module portion of the spec, but HostImportModuleDynamically seems pretty telling.

and I'm not sure how it would be exposed anyway, since both can happen to the same module

Yes, but the module will only be evaluated once.

[...] a host has access to its own module graph

Sure, again thinking along the lines of module attributes, if a module is imported in more than one place with different sets of attribtues, I guess it would have a unique module record, so that should be sufficient for this use case. 👍

Modules have a [[HostDefined]] slot [...]

Ah, the [[HostDefined]] slot perfectly fits the last use case. 👍

@devsnek
Copy link
Member Author

devsnek commented Mar 13, 2020

@michaelficarra HostImportModuleDynamically paraphrased is "at some point in the future do your host specific magic and then call FinishDynamicImport" and FinishDynamicImport just the static logic plus fulfilling or rejecting a promise.

Beyond that, your pattern idea here would seem to imply that one might do import x from 'x' and be given something meant only for a dynamic consumer, or import('x') and be given something meant only for a static consumer (and I have zero ideas for what kind of logic/data would need to be switched based on that anyway).

@devsnek
Copy link
Member Author

devsnek commented Mar 13, 2020

Also to be clear about the above, part of the "host specific magic" could be setting data in [[HostDefined]] which says whether it was static or dynamic.

@michaelficarra
Copy link
Member

Okay I think I'm satisfied that the module record alone should be sufficient for these host hooks. Thanks for the explanations, @devsnek. I still don't think we should be adding two overlapping hooks for this feature, though.

spec.html Outdated

<p>The implementation of HostFinalizeImportMeta must conform to the following requirements:</p>
<ul>
<li>It must always complete normally (i.e., not return an abrupt completion).</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably say more about what it can do to the importMeta object that's passed in? It shouldn't, e.g., transmute it into an exotic object by mucking with the internal methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds like that might need to be a more general forbidden extension then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the specific case of transmutation into an exotic sounds like a good general forbidden extension to have. Did we discuss the general invariants of this host hook?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think as long as this hook doesn't make the object exotic, anything is fair game, including changing [[Prototype]].

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think as long as this hook doesn't make the object exotic, anything is fair game, including changing [[Prototype]].

That seems reasonable. Is this captured in the notes somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

@syg i couldn't find anything as explicit as my sentence above, but between issues and notes and light discussion with domenic that seems to be the understanding.

@syg syg added the editor call to be discussed in the next editor call label Mar 13, 2020
@syg
Copy link
Contributor

syg commented Mar 13, 2020

I still don't think we should be adding two overlapping hooks for this feature, though.

Layering wise there are some advantages in providing both low- and high-expressivity hooks. When reasoning about layered specs, if we know only the low-expressivity hook is implemented by the host spec, that's easier to reason about, right? Since HTML only uses the non-escape hatch, we don't have to really read HTML's implementation of the hook to know import.meta is going to remain a proto-ess ordinary object with only data properties.

Let's chat about this on the next call.

@devsnek
Copy link
Member Author

devsnek commented Mar 13, 2020

^ To this point, I'm not aware of any engines (except engine262) which implement both hooks, they just implement the finalize one.

@syg
Copy link
Contributor

syg commented Mar 13, 2020

^ To this point, I'm not aware of any engines (except engine262) which implement both hooks, they just implement the finalize one.

Yeah, that seems expected. My point was about reasoning about host specs, not so much implementations.

jmdyck
jmdyck previously requested changes Mar 22, 2020
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@michaelficarra michaelficarra removed the editor call to be discussed in the next editor call label Mar 25, 2020
ljharb pushed a commit to devsnek/ecma262 that referenced this pull request Mar 31, 2020
Co-Authored-By: Domenic Denicola <d@domenic.me>
Co-Authored-By: Sathya <gsathya.ceg@gmail.com>
Co-Authored-By: Peter Robins <github@peterrobins.co.uk>
Co-Authored-By: Gus Caplan <me@gus.host>
@ljharb ljharb dismissed jmdyck’s stale review March 31, 2020 22:31

concerns addressed

@ljharb ljharb requested review from michaelficarra, syg and a team March 31, 2020 22:31
@ljharb ljharb requested a review from bakkot March 31, 2020 22:31
spec.html Show resolved Hide resolved
Co-Authored-By: Domenic Denicola <d@domenic.me>
Co-Authored-By: Sathya <gsathya.ceg@gmail.com>
Co-Authored-By: Peter Robins <github@peterrobins.co.uk>
Co-Authored-By: Gus Caplan <me@gus.host>
@devsnek devsnek added has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. and removed pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. labels Apr 1, 2020
@ljharb ljharb self-assigned this Apr 2, 2020
@ljharb ljharb merged commit 3c6dbe6 into tc39:master Apr 2, 2020
@devsnek devsnek deleted the import-meta branch April 2, 2020 02:39
@mysticatea mysticatea mentioned this pull request Apr 2, 2020
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants