src,lib: implement import.meta #18368

targos · 2018-01-25T09:56:41Z

The feature is still behind the --harmony-import-meta V8 flag.
This commit implements the C++ callback that is required to configure
the import.meta object and adds ~~two properties~~ one property:

url: absolute URL of the module
~~- require: CommonJS require function~~

/cc @nodejs/esm @MylesBorins @nodejs/tsc

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

ESM

vsemozhetbyt · 2018-01-25T11:27:40Z

doc/api/esm.md

+
+Available with the flag `--harmony-import-meta`, the `import.meta` metaproperty
+is an `Object` that contains the following properties:
+* `url` {String} The absolute `file:` URL of the module


A nit: we usually lowercase primitive types in docs (i.e. {string})

guybedford · 2018-01-25T14:21:15Z

Is the idea with import.meta.require to then implement defaulting ".js" loads from ES modules as being treated as ES modules themselves? I think we should be clear exactly what that proposal is, so it can be adequately reviewed. It could also make sense to separate this out into a separate PR with the full proposal to help focus discussion.

cjihrig · 2018-01-25T14:12:12Z

doc/api/esm.md

+Available with the flag `--harmony-import-meta`, the `import.meta` metaproperty
+is an `Object` that contains the following properties:
+* `url` {string} The absolute `file:` URL of the module
+* `require` {Function} The same `require` function that is usually available in


Can you link to the require() docs here.

bmeck · 2018-01-25T14:47:58Z

@targos can we split out import.meta.require for now since thats a bigger talk than the non-controversial import.meta.url

targos · 2018-01-25T15:17:16Z

@bmeck done. I kept the JS initialization function for later use.

guybedford

👍

targos · 2018-01-25T15:40:34Z

CI: https://ci.nodejs.org/job/node-test-commit/15681/

ljharb

Can this add some tests that import.meta.nothing is an error, as well as import.meta.toString?

bmeck · 2018-01-25T18:38:02Z

@ljharb is the following check fine?

assert.stictEqual(Object.getPrototypeOf(import.meta), null);
assert.deepEqual(Object.keys(Object.getOwnPropertyDescriptors(import.meta)), ['url']);

devsnek · 2018-01-25T18:50:52Z

src/module_wrap.cc

+                                            Local<Object> meta) {
+  Isolate* isolate = context->GetIsolate();
+  Environment* env = Environment::GetCurrent(isolate);
+  meta->CreateDataProperty(context, env->url_string(),


i'm wondering if there's any perf hit with moving this to the js callback. if there isn't i would suggest doing that just for consistency.

You mean by passing the URL string to the JS callback?

no i mean like

importMetaCallback(wrap, meta) { // attach wrap.url to meta in a safe way here }

ljharb · 2018-01-25T18:51:56Z

@bmeck should probably be gOPDs, but sure, that sounds great!

bmeck · 2018-01-25T19:26:48Z

test/es-module/test-esm-import-meta.mjs

+import assert from 'assert';
+
+const urlReg = /^file:\/\/\/.*\/test\/es-module\/test-esm-import-meta\.mjs$/;
+assert(import.meta.url.match(urlReg));


per #18368 (review) we can add

assert.stictEqual(Object.getPrototypeOf(import.meta), null); assert.deepEqual(Object.keys(Object.getOwnPropertyDescriptors(import.meta)), ['url']);

Could the second line use Reflect.ownKeys(import.meta) instead? Or am I missing something?

change ['urls'] to [{ enumerable: true, writable: false, configurable: false, value: 'url' }], eg

Reflect.ownKeys is good because it also covers symbols; but it doesn't cover descriptors.

MylesBorins · 2018-01-25T22:01:52Z

As import.meta will require a flag in V8 to turn on. Does it make sense to make the --experimental-modules flag also flip the appropriate V8 flag?

devsnek · 2018-01-25T22:03:52Z

probs not, we don't do it for import() and the features are still experimental above node's proverbial head.

MylesBorins · 2018-01-25T22:05:03Z

@devsnek I plan to open a PR to do the same for import() fwiw

edit: both features are shipping in chrome with the flag turned off... the flag currently is only turned on in V8 to embedders from having the apis called without the callback being set (would have an unhandled rejection)

addaleax · 2018-01-25T22:53:06Z

src/module_wrap.cc

+void ModuleWrap::HostInitializeImportMetaObjectCallback(
+    Local<Context> context, Local<Module> module, Local<Object> meta) {
+  Isolate* isolate = context->GetIsolate();
+  Environment* env = Environment::GetCurrent(isolate);


Tiny nit: Can you use Environment::GetCurrent(context)? Getting it from the isolate only looks up the context on the isolate and takes it from there

Ah thanks! I didn't know there was a Environment::GetCurrent(context)

Fishrock123 · 2018-01-26T15:53:00Z

Can we add meta.dirname, meta.filename, and meta.isMain?

bmeck · 2018-01-26T16:01:33Z

@Fishrock123 you can get dirname and filename from the url, and they get normalized always when they go through becoming a URL (thank goodness). I'm neutral to isMain but feel like thare are various ways to figure it out as well, just nothing convenient like the dirname or filename solution.

Fishrock123 · 2018-01-26T16:07:50Z

you can get dirname and filename from the url

I'm not really sure that means we should force users to always parse the url when there is already a precedent set here.

I'd love to know how you can figure out inMain without doing it internally, but we'll still need it if doing so is difficult.

devsnek · 2018-01-26T16:15:15Z

personally i'm -1 to dirname/filename. you have your url which is generally enough and to be honest the cases where you need the filename of the file you wrote are imo generally rare enough to warrant you having to parse the url yourself.

bmeck · 2018-01-26T16:15:19Z

@Fishrock123

I'm not really sure that means we should force users to always parse the url when there is already a precedent set here.

The precedent exists but normalization of \\ to / simplifies things and fixes some common cross platform bugs, also .url is compatible between both Node and browser environments. Even if we supplied .filename there are also some problems of passing it around to import() etc. if you intended to preserve query and hash fragments. I see history in .dirname / .filename but don't think that is enough when the workflow exists with .url and avoids some compatibility both with OS and host environment considerations.

I'd love to know how you can figure out inMain without doing it internally, but we'll still need it if doing so is difficult.

Parsing out process.argv/process.cwd() vs the location identifier, we used to do this way back when at Nodejitsu and the technique still works. Very ugly to do though. Has a nice benefit of wrappers being able to setup a new "main" entry point without spawning a new process though.

bmeck · 2018-01-29T21:27:25Z

@jdalton the main attraction to frozen things like that is that you can't bail out of static analysis if it becomes mutable. Bailing out is probably fine if people have to do it for browsers anyway I think, but still a little bit of a question on who is actively wanting to rewrite .url and why instead of doing some source code transform.

ljharb · 2018-01-29T21:30:10Z

@jdalton I just said it'd be unfortunate; not that i'd "fight" for it. I could ask the same question of browsers - why would they ship a fully unlocked descriptor without consulting with node first? "Interoperable" could also mean that browsers ship a non-writable non-enumerable descriptor, for example.

jdalton · 2018-01-29T21:30:13Z

@bmeck

The main attraction to frozen things like that is that you can't bail out of static analysis if it becomes mutable.

I think it's totally fine to bring those things up as additions or changes to proposals/specs/browsers-early-implementations. However, the ship has kinda sailed on the import.meta.url front for now and the PR should align with how it is in browser-land. If it's changed later, after spec/browser discussions or whatever, then great!

TimothyGu · 2018-01-29T21:31:54Z

src/module_wrap.cc

+  ModuleWrap* module_wrap = ModuleWrap::GetFromModule(env, module);
+
+  if (module_wrap == nullptr) {
+    env->ThrowError("ModuleWrap object not found in map");


How about we just return and leave the object empty? The upcoming vm.Module PR will make it possible to have Modules that are not in the core module map.

Can't the vm.Module code just delete the property if it is configurable?

Yes, but this line throws an error. We don’t want that.

I will remove the throw, but we will probably have to come up with a solution to configure import.meta in vm.Module

targos · 2018-01-30T09:45:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12811/

jdalton · 2018-01-31T09:42:00Z

@targos Just curious, can you check that eval("import.meta") errors in ESM.

For example in Chrome:

eval("import.meta")
// throws "SyntaxError: Cannot use 'import.meta' outside a module

targos · 2018-01-31T10:04:48Z

@jdalton This is already tested by V8:

node/deps/v8/test/mjsunit/harmony/modules-import-meta.js

Line 39 in a025723

assertThrows(() => eval('import.meta'), SyntaxError);

jdalton · 2018-01-31T15:29:58Z

@targos Yep. I know Node had to do some extra work to make static import errors more clear, so I was curious if things were OK for import.meta.

targos · 2018-01-31T15:46:45Z

@jdalton Here is the output:

$ cat test.mjs

eval("import.meta");


$ ./node --experimental-modules test.mjs
(node:16949) ExperimentalWarning: The ESM module loader is experimental.
SyntaxError: Cannot use 'import.meta' outside a module
    at file:///home/mzasso/git/nodejs/node/test.mjs:2:1
    at ModuleJob.run (internal/loader/ModuleJob.js:106:14)
    at <anonymous>

targos · 2018-01-31T15:49:06Z

I had to rebase because of a conflict.
New CI: https://ci.nodejs.org/job/node-test-pull-request/12847/

jdalton · 2018-01-31T16:04:08Z

Here is the output:

Thanks! It looks like it's missing the arrow stack decorator.

TimothyGu · 2018-02-01T00:20:22Z

Other than the conflict, is there anything preventing this from getting merged?

Implement the C++ callback that is required to configure the `import.meta` object and add one property: - url: absolute URL of the module

BridgeAR · 2018-02-01T10:09:22Z

One more CI before landing: https://ci.nodejs.org/job/node-test-pull-request/12877/

BridgeAR · 2018-02-01T12:47:17Z

Landed in 9e1a6f8

Implement the C++ callback that is required to configure the `import.meta` object and add one property: - url: absolute URL of the module PR-URL: nodejs#18368 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

targos added dont-land-on-v4.x esm Issues and PRs related to the ECMAScript Modules implementation. labels Jan 25, 2018

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 25, 2018

vsemozhetbyt reviewed Jan 25, 2018

View reviewed changes

cjihrig reviewed Jan 25, 2018

View reviewed changes

guybedford approved these changes Jan 25, 2018

View reviewed changes

bmeck approved these changes Jan 25, 2018

View reviewed changes

cjihrig approved these changes Jan 25, 2018

View reviewed changes

ljharb suggested changes Jan 25, 2018

View reviewed changes

devsnek reviewed Jan 25, 2018

View reviewed changes

bmeck reviewed Jan 25, 2018

View reviewed changes

addaleax approved these changes Jan 25, 2018

View reviewed changes

TimothyGu reviewed Jan 29, 2018

View reviewed changes

targos force-pushed the import-meta branch from 746d596 to 63a3a9f Compare January 30, 2018 08:35

TimothyGu approved these changes Jan 30, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 31, 2018

MylesBorins mentioned this pull request Jan 31, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-01-31 nodejs/TSC#478

Closed

targos force-pushed the import-meta branch from 63a3a9f to 813e363 Compare January 31, 2018 15:47

src,lib: implement import.meta

21ce2c5

Implement the C++ callback that is required to configure the `import.meta` object and add one property: - url: absolute URL of the module

targos force-pushed the import-meta branch from 813e363 to 21ce2c5 Compare February 1, 2018 09:02

BridgeAR closed this Feb 1, 2018

targos deleted the import-meta branch February 1, 2018 12:56

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2018

guybedford mentioned this pull request Feb 22, 2018

esm: Add support for pjson cache and main without extensions #18728

Closed

4 tasks

src,lib: implement import.meta #18368

src,lib: implement import.meta #18368

Conversation

targos commented Jan 25, 2018 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

guybedford commented Jan 25, 2018

Choose a reason for hiding this comment

bmeck commented Jan 25, 2018

targos commented Jan 25, 2018

guybedford left a comment

Choose a reason for hiding this comment

targos commented Jan 25, 2018

ljharb left a comment

Choose a reason for hiding this comment

bmeck commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Jan 25, 2018

Choose a reason for hiding this comment

apapirovski Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MylesBorins commented Jan 25, 2018

devsnek commented Jan 25, 2018

MylesBorins commented Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Jan 26, 2018

bmeck commented Jan 26, 2018

Fishrock123 commented Jan 26, 2018

devsnek commented Jan 26, 2018

bmeck commented Jan 26, 2018 • edited Loading

bmeck commented Jan 29, 2018

ljharb commented Jan 29, 2018

jdalton commented Jan 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Jan 30, 2018

jdalton commented Jan 31, 2018 • edited Loading

targos commented Jan 31, 2018

jdalton commented Jan 31, 2018 • edited Loading

targos commented Jan 31, 2018

targos commented Jan 31, 2018

jdalton commented Jan 31, 2018 • edited Loading

TimothyGu commented Feb 1, 2018

BridgeAR commented Feb 1, 2018

BridgeAR commented Feb 1, 2018

targos commented Jan 25, 2018 •

edited

Loading

apapirovski Jan 25, 2018 •

edited

Loading

MylesBorins commented Jan 25, 2018 •

edited

Loading

bmeck commented Jan 26, 2018 •

edited

Loading

jdalton commented Jan 29, 2018 •

edited

Loading

jdalton commented Jan 31, 2018 •

edited

Loading

jdalton commented Jan 31, 2018 •

edited

Loading

jdalton commented Jan 31, 2018 •

edited

Loading