Till Schneidereit [:till] Reporter
	Description • 6 years ago

The import.meta proposal is at stage 3, and shipping in stable Chrome since Chrome 64. We should aim to ship this when we ship modules on stable at all.

	Till Schneidereit [:till] Reporter
	Comment 1 • 6 years ago

Jon, are you planning to have this ship as part of our initial ES6 modules implementation? The spec is definitely stable enough for us to ship it to release, and it'd be a valuable feature to have from the get-go.

	Jon Coppeard (:jonco) Assignee
	Comment 2 • 6 years ago

(In reply to Till Schneidereit [:till] PTO February 6 - 16 from comment #1)
I wasn't planning on shipping this as part of the initial implementation, but I'd like to work on it straight after.  Do you think it's worth making this block the initial implementation?

	Till Schneidereit [:till] Reporter
	Comment 3 • 6 years ago

I agree that this doesn't have to block initial implementation. Which is a good thing, given that you've since flipped the switch to let modules ride the trains :)

Given that the contents of import.meta are implementation defined, one reason for getting this in sooner rather than later is to ensure that there are multiple implementations and hence a reason to coordinate what goes into import.meta. I don't know what the Chrome team's plans around this are, but we should ensure that we have a dialog with them about this.

	Justin Fagnani
	Comment 4 • 6 years ago

Any updates on when this might be ship wrt basic module support?

On the Polymer project we're about to publish 3.0 using native modules that rely on dynamic import() and import.meta.url. In our tools we'd like to just say that module support includes dynamic import() and import.meta.url to reduce the build permutations we have to support. So far Chrome and Safari Technology Preview support both.

	Tom S [:evilpie]
	Comment 5 • 6 years ago

We are going to ship modules, without dynamic import() and import.meta. Jon can probably comment on when we plan on implementing those features.

	Domenic Denicola
	Comment 6 • 6 years ago

FWIW we have tests at http://w3c-test.org/html/semantics/scripting-1/the-script-element/module/import-meta/import-meta-url.html ready to go.

	Jon Coppeard (:jonco) Assignee
	Comment 7 • 6 years ago

Patch to implement parsing of |import.meta|.

Currently there is no state in the parser to say whether we were parsing for the script goal or the module goal (we currently only need to know whether we are at the top level of a module), so this adds that state and propagates it through full parsing of lazy scripts.

Mostly import.meta is handled similarly to new.target.

One complication was the statement processing where an 'import' keyword can be the start of an import declaration or an import.meta expression.  I added importDeclarationOrImportMeta() methods to handle this.  I'm not sure if this is the best way to do it though.

	Jon Coppeard (:jonco) Assignee
	Comment 8 • 6 years ago

Implement import.meta in the JS shell.  This adds a slot to module objects to hold the meta object.  We get the module associated with a script by walking the scope chain.  There is a single hook for the embedding to perform the role of both the HostGetImportMetaProperties and HostFinalizeImportMeta operations.

	Jon Coppeard (:jonco) Assignee
	Comment 9 • 6 years ago

JIT support.  If we baseline compile a script that uses import.meta we create the meta object at that time for the sake of simplicity.  For Ion we assume that this has been done by baseine.

	Jon Coppeard (:jonco) Assignee
	Comment 10 • 6 years ago

Set up module metadata hook to implmement import.meta for modules in the browser.  This just defines the 'url' property.

HTML spec is here: https://html.spec.whatwg.org/multipage/webappapis.html#hostgetimportmetaproperties

	André Bargull [:anba]
	Comment 11 • 6 years ago

Comment on attachment 8976589 [details] [diff] [review]
bug1427610-2-implement-import-meta-shell

Review of attachment 8976589 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

::: js/src/builtin/ModuleObject.cpp
@@ +1645,5 @@
> +
> +JSObject*
> +js::GetOrCreateModuleMetaObject(JSContext* cx, HandleObject moduleArg)
> +{
> +    RootedModuleObject module(cx, &moduleArg->as<ModuleObject>());

`Handle<ModuleObject*> module = moduleArg.as<ModuleObject>();` so we don't need to root it twice.

@@ +1651,5 @@
> +    JSObject* obj = module->metaObject();
> +    if (obj)
> +        return obj;
> +
> +    obj = cx->runtime()->moduleMetaObjectHook(cx, module);

This is a bit more permissive than the spec proposal, because it also allows the embedding to return an exotic object (i.e. not a js::PlainObject). This is probably not an issue as long as it isn't misused by the embedder.

(If we create the object here and then pass it to the embedding, we could ensure that the object is always in the tenured heap, because maybe it's preferable to have a tenured object when baking it into the jit-code. Please double-check this with jandem for Part 3!)

The spec proposal also requires that this hook never returns an abrupt completion. I don't think we can (b/c of OOM) and should have these requirements in our implementation, so handling the case when |obj| is nullptr here seems correct to me.

::: js/src/builtin/ModuleObject.h
@@ +253,5 @@
>          EnvironmentSlot,
>          NamespaceSlot,
>          StatusSlot,
>          EvaluationErrorSlot,
> +        MetaObjectSlot,

The last slot before regressing bug 1420412! :-)

::: js/src/jit-test/tests/modules/import-meta.js
@@ +1,4 @@
> +// |jit-test| module
> +
> +// import.meta is an object.
> +assertEq(typeof import.meta, "object");

Maybe additionally `assertEq(import.meta !== null, true);`, because |typeof null === "object"|.

@@ +24,5 @@
> +let otherImportMeta = getOtherMetaObject();
> +assertEq(otherImportMeta.url.endsWith("exportImportMeta.js"), true);
> +
> +// By default the import.meta object will be extensible, and its properties will
> +// be writable, configurable, and enumerable.

Maybe add an explicit test for this?

```
assertEq(Object.isExtensible(import.meta), true);

var desc = Object.getOwnPropertyDescriptor(import.meta, "url");
assertEq(desc.writable, true);
assertEq(desc.enumerable, true);
assertEq(desc.configurable, true);
assertEq(desc.value, import.meta.url);
```


And also add a test for the prototype?
```
assertEq(Object.getPrototypeOf(import.meta), null);
```

@@ +34,5 @@
> +assertEq(import.meta.newProp, 42);
> +
> +let found = new Set();
> +for (let p in import.meta)
> +    found.add(p);

Better: `let found = new Set(Reflect.ownKeys(import.meta));`

That way symbols and non-enumerable properties are included, too.

@@ +45,5 @@
> +delete import.meta.newProp;
> +
> +found = new Set();
> +for (let p in import.meta)
> +    found.add(p);

Also here: `let found = new Set(Reflect.ownKeys(import.meta));`

::: js/src/shell/js.cpp
@@ +4312,5 @@
>  
> +static JSObject*
> +ShellModuleMetaObjectHook(JSContext* cx, HandleObject module)
> +{
> +    RootedObject obj(cx, JS_NewPlainObject(cx));

|JS_NewObjectWithGivenProto(cx, nullptr, nullptr)| to match the default prototype in the spec proposal ("Set importMeta to ObjectCreate(null).").

@@ +4318,5 @@
> +        return nullptr;
> +
> +    // For the shell, just use the script's filename as the base URL.
> +    RootedScript script(cx, module->as<ModuleObject>().script());
> +    const char* filename = script->scriptSource()->filename();

Is |filename| guaranteed to be non-null here? For example when the module is manually constructed using the shell test-API?

@@ +4323,5 @@
> +    RootedString url(cx, NewStringCopyZ<CanGC>(cx, filename));
> +    if (!url)
> +        return nullptr;
> +
> +    JS_DefineProperty(cx, obj, "url", url, JSPROP_ENUMERATE);

if (!JS_DefineProperty(...))
    return nullptr;

to handle possible OOM.

::: js/src/vm/EnvironmentObject.h
@@ +1169,1 @@
>  ModuleEnvironmentObject* GetModuleEnvironmentForScript(JSScript* script);

Nit: Blank line between both functions.

::: js/src/vm/Interpreter.cpp
@@ +4213,5 @@
>  END_CASE(JSOP_NEWTARGET)
>  
> +CASE(JSOP_IMPORTMETA)
> +{
> +    ReservedRooted<JSObject*> module(&rootObject0, GetModuleObjectForScript(script));

Add `MOZ_ASSERT(module);` because GetOrCreateModuleMetaObject(...) expects a non-null argument, but GetModuleObjectForScript(...) has an explicit `return nullptr;`?

@@ +4214,5 @@
>  
> +CASE(JSOP_IMPORTMETA)
> +{
> +    ReservedRooted<JSObject*> module(&rootObject0, GetModuleObjectForScript(script));
> +    JSObject* metaObject = GetOrCreateModuleMetaObject(cx, module);

Is it possible to use `module.as<ModuleObject>()` here and then change GetOrCreateModuleMetaObject to only accept `Handle<ModuleObject*>` for better type safety? Or are they some restrictions because of reserved rooted?

::: js/src/vm/Opcodes.h
@@ +2348,5 @@
> +    /*
> +     * Push "import.meta"
> +     *
> +     *   Category: Variables and Scopes
> +     *   Type: Arguments

Hmm, I don't think "Arguments" is correct to reuse here. Maybe just add a new "Type: Module"?

	Jan de Mooij [:jandem]
	Comment 12 • 6 years ago

Comment on attachment 8976652 [details] [diff] [review]
bug1427610-3-jit-support

Review of attachment 8976652 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

	Jon Coppeard (:jonco) Assignee
	Comment 13 • 6 years ago

Thanks for all the comments!

I changed the hook so that object is created first and then passed in, which is a bit safer as you touched on in your comments.

> Is |filename| guaranteed to be non-null here? For example when the module is manually constructed using the shell test-API?

Yes, this ends up being "<string>" in that case, but I added an assert.

> Is it possible to use `module.as<ModuleObject>()` here and then change GetOrCreateModuleMetaObject to only accept `Handle<ModuleObject*>` for better type safety? Or are they some restrictions because of reserved rooted?

This didn't work because of |module| being a reserved rooted.

Other comments addressed.

	André Bargull [:anba]
	Comment 14 • 6 years ago

Comment on attachment 8976937 [details] [diff] [review]
bug1427610-2-implement-import-meta-shell

Review of attachment 8976937 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, still looks good!

::: js/src/builtin/ModuleObject.cpp
@@ +1649,5 @@
> +    HandleModuleObject module = moduleArg.as<ModuleObject>();
> +    if (JSObject* obj = module->metaObject())
> +        return obj;
> +
> +    RootedObject metaObject(cx, JS_NewObjectWithGivenProto(cx, nullptr, nullptr));

Nit: Now that we're allocating the object in the js-engine, we should directly call |NewObjectWithGivenProto<PlainObject>(cx, nullptr);| instead of going through JSAPI.

::: js/src/vm/Runtime.h
@@ +941,5 @@
>      js::MainThreadData<JS::ModuleResolveHook> moduleResolveHook;
>  
> +    // A hook that implements the abstract operations
> +    // HostGetImportMetaProperties and HostFinalizeImportMeta.
> +    js::MainThreadData<JS::ModuleMetadataHook> moduleMetadataHook;

Do we need to explicitly initialize the field in the constructor? Asking because I just saw that |moduleResolveHook| is explicitly initialized here <https://searchfox.org/mozilla-central/rev/da499aac682d0bbda5829327b60a865cbc491611/js/src/vm/Runtime.cpp#178>.

	Jon Coppeard (:jonco) Assignee
	Comment 15 • 6 years ago

Updated patch following review feedback above.  Sorry for the churn.

	Ben Kelly [:bkelly, not reviewing]
	Comment 16 • 6 years ago

Comment on attachment 8976957 [details] [diff] [review]
bug1427610-4-script-loader

Review of attachment 8976957 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/script/ScriptLoader.cpp
@@ +809,5 @@
> +                       JS::Handle<JSObject*> aMetaObject)
> +{
> +  JS::Value value = JS::GetModuleHostDefinedField(aModule);
> +  auto script = static_cast<ModuleScript*>(value.toPrivate());
> +  MOZ_ASSERT(script->ModuleRecord() == aModule);

Can you make this a MOZ_DIAGNOSTIC_ASSERT() since its a cheap comparison?

Also, would it be worth MOZ_DIAGNOSTIC_ASSERT'ing that aModule is not nullptr.

@@ +812,5 @@
> +  auto script = static_cast<ModuleScript*>(value.toPrivate());
> +  MOZ_ASSERT(script->ModuleRecord() == aModule);
> +
> +  nsAutoCString url;
> +  script->BaseURL()->GetAsciiSpec(url);

I think you need to check for BaseURL() being nullptr since its a cycle collected member.  It can get unlinked while the ModuleScript is still alive.

As a side note, is there any impact to GC/CC from storing the ModuleScript in the metadata hook mechanism?  That seems to be defined in another patch and I'm not sure how that impact the lifecycle.

Also, if we are going to assume GetAsciiSpec() is infallible, then please wrap the call in MOZ_ALWAYS_SUCCEEDS() to assert that in debug builds.

	Jason Orendorff [:jorendorff]
	Comment 17 • 6 years ago

Comment on attachment 8976171 [details] [diff] [review]
bug1427610-1-parse-import-meta

Review of attachment 8976171 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/Parser.cpp
@@ +8726,5 @@
>              return null();
>          lhs = handler.newSuperBase(thisName, pos());
>          if (!lhs)
>              return null();
> +

Style nit: no blank line here.

	Jon Coppeard (:jonco) Assignee
	Comment 18 • 6 years ago

(In reply to Ben Kelly [:bkelly] from comment #16)
> As a side note, is there any impact to GC/CC from storing the ModuleScript
> in the metadata hook mechanism?  That seems to be defined in another patch
> and I'm not sure how that impact the lifecycle.

The ModuleScript has a pointer to the JS ModuleObject which acts as a strong reference and keeps the ModuleObject alive as long as the ModuleScript is live.  The ModuleObject has a pointer back to the ModuleScript set with SetModuleHostDefinedField() which acts like a weak pointer and doesn't keep the ModuleScript alive (we don't increment its ref count when this is set).  When the ModuleScript is unlinked we clear this back pointer.  So this doesn't influence the lifetime of the ModuleScript.

I'll add a diagnostic assert that the back pointer is set.  It's not possible to still be running scripts after the ScriptLoader has died, right?

	Jon Coppeard (:jonco) Assignee
	Comment 19 • 6 years ago

(In reply to Jon Coppeard (:jonco) from comment #18)
> I'll add a diagnostic assert that the back pointer is set.

On second thoughts, I'll play it safe and report an error if this is not set.

	Pulsebot
	Comment 20 • 6 years ago

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/277bd9cf9edc
Implement import.meta in the JS frontent r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/a506ea1db794
Implement import.meta in the JS shell r=anba
https://hg.mozilla.org/integration/mozilla-inbound/rev/847453f52aab
Support import.meta in the JITs r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5e8a990f0a
Implement import.meta in the browser r=bkelly

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 21 • 6 years ago

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/11117 for changes under testing/web-platform/tests

	Andreea Pavel [:apavel]
	Comment 22 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/277bd9cf9edc
https://hg.mozilla.org/mozilla-central/rev/a506ea1db794
https://hg.mozilla.org/mozilla-central/rev/847453f52aab
https://hg.mozilla.org/mozilla-central/rev/9a5e8a990f0a

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 23 • 6 years ago

Upstream PR merged

	Florian Scholz (Open Web Docs)
	Comment 24 • 6 years ago

Announced on Firefox 62 for developers:
https://developer.mozilla.org/en-US/Firefox/Releases/62#JavaScript

New page added:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import.meta

Compat data update:
https://github.com/mdn/browser-compat-data/pull/2141

:jonco (or :till), can you let me know if these docs look alright to you?

The spec has a neat example (https://github.com/tc39/proposal-import-meta#example) using import.meta.scriptElement, but that's not implemented yet, is it? Do we have a bug for it?

Thank you!

	Jon Coppeard (:jonco) Assignee
	Comment 25 • 6 years ago

(In reply to Florian Scholz [:fscholz] (MDN) from comment #24)

> The spec has a neat example
> (https://github.com/tc39/proposal-import-meta#example) using
> import.meta.scriptElement, but that's not implemented yet, is it? Do we have
> a bug for it?

That's just an example to justify the proposed introduction of import.meta as a feature.  There is no spec for adding this particular property.  The html spec defines only the 'url' property:

https://html.spec.whatwg.org/multipage/webappapis.html#hostgetimportmetaproperties

From the MDN page:

"It contains information about the module, like the path of the directory in which the current module is stored and other meta data"

As above, it only contains the URL.

Apart from that it looks fine.

	Florian Scholz (Open Web Docs)
	Comment 26 • 6 years ago

Awesome, thank you Jon! :)