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

module: Allow runMain to be ESM #14369

Closed
wants to merge 1 commit into from
Closed

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Jul 19, 2017

This follows the EPS an allows the node CLI to have ESM as an entry point.
node ./example.mjs. A newer V8 is needed for import() so that is not
included. import.meta is still in specification stage so that also is not
included.

Author: Bradley Farias [email protected]
Author: Guy Bedford [email protected]
Author: Jan Krems [email protected]

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)

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jul 19, 2017
@bmeck
Copy link
Member Author

bmeck commented Jul 19, 2017

Going to be on vacation til TC39 next week / tests need to be pulled in from sketchy. Opening this since it is going to want a lot of eyes.

@jasnell
Copy link
Member

jasnell commented Jul 19, 2017

Great to see progress here. Won't have an opportunity to review likely until after NodeSummit next week.

Copy link
Member

@TimothyGu TimothyGu left a comment

Choose a reason for hiding this comment

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

More to come later. Is the C++ resolution necessary or for performance? I kinda drew the line at parsing JSON in C++...

}

async createModule() {
const source = `${await asyncReadFile(this.url.pathname)}`;
Copy link
Member

Choose a reason for hiding this comment

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

There's a specialized function you could use instead of this.url.pathname.

`${this.url}`);
}
}
Object.setPrototypeOf(StandardModuleRequest.prototype, Object.create(null));
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? If so, why not just set the prototype to null?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an encapsulation general thing so that it does not inherit from Object so cannot be affected by mutated globals. In line with #10135

Copy link
Member

@TimothyGu TimothyGu Jul 26, 2017

Choose a reason for hiding this comment

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

Yeah I get that, but you don't need to set the prototype object to an object with prototype null, you can just set the prototype of the prototype object to null.

A chart for easier consumption of what I just wrote:

Current:

| StandardModuleRequest.prototype |
               ||    [[Prototype]]
               vv
     | Object.create(null) |
               ||    [[Prototype]]
               vv
            | null |


Why not:

| StandardModuleRequest.prototype |
               ||    [[Prototype]]
               vv
            | null |

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, yes. we should change that.

const ModuleMap = require('internal/loader/ModuleMap');
const ModuleJob = require('internal/loader/ModuleJob');
const resolveRequestUrl = require('internal/loader/resolveRequestUrl');
const {Error, Promise: {resolve: PromiseResolve}} = global;
Copy link
Member

Choose a reason for hiding this comment

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

Is this (and safe_globals and other such things) necessary?

@@ -0,0 +1,24 @@
const copyProps = (unsafe, safe) => {
Copy link
Member

Choose a reason for hiding this comment

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

If this is necessary, 'use strict'.

Copy link
Member Author

Choose a reason for hiding this comment

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

was fixed, forgot to reply

const {URL} = require('url');
const internalCJSModule = require('internal/module');
const NativeModule = require('native_module');
const Path = require('path');
Copy link
Member

Choose a reason for hiding this comment

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

Why not use destructuring to prevent monkey patching as has been done elsewhere? Either way s/P/p/.

return;
}

const Local<Value> ret(result.ToObject(
Copy link
Member

Choose a reason for hiding this comment

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

Why not just ret =?

Copy link
Member

Choose a reason for hiding this comment

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

@TimothyGu I agree with your comments here, but it might be easiest to just do what @targos did and push changes, esp. for suggestions like these, yourself

Copy link
Member

Choose a reason for hiding this comment

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

@addaleax 👍

}

using namespace node::url;
URL __init_cwd() {
Copy link
Member

Choose a reason for hiding this comment

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

Weird identation...

else if (specifier[0] == '.') {
if (len == 1 || specifier[1] == '/') return true;
else if (specifier[1] == '.') {
if (len == 2 || specifier[2] == '/') return true;
Copy link
Member

Choose a reason for hiding this comment

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

Does this support Windows?

Copy link
Member

Choose a reason for hiding this comment

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

return URL("");
}
URL Resolve(std::string specifier, URL* base, bool read_pkg_json) {
auto pure_url = URL(specifier.c_str());
Copy link
Member

Choose a reason for hiding this comment

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

URL takes a std::string as input.

using v8::EscapableHandleScope;
using v8::PropertyDescriptor;

const std::string EXTENSIONS[] = {".mjs", ".js", ".json", ".node"};
Copy link
Member

@TimothyGu TimothyGu Jul 20, 2017

Choose a reason for hiding this comment

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

Did we reach a conclusion on the non-Web-compatible automatic extension adding behavior?

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

This is a very complex feature to review without any attached unit tests. Would you mind adding them?

@targos
Copy link
Member

targos commented Jul 20, 2017

I took the liberty to fix ESLint issues and compilation.
I don't think this is ready to be reviewed:

make test
$ make test
make -C out BUILDTYPE=Release V=1
  touch 9349840c76d301fd95cb4ac8c5f455128549de34.intermediate
  LD_LIBRARY_PATH=/home/mzasso/git/nodejs/node/out/Release/lib.host:/home/mzasso/git/nodejs/node/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../deps/v8/src/inspector; mkdir -p /home/mzasso/git/nodejs/node/out/Release/obj/gen/src/inspector/protocol /home/mzasso/git/nodejs/node/out/Release/obj/gen/include/inspector; python ../../third_party/inspector_protocol/CodeGenerator.py --jinja_dir ../../third_party --output_base "/home/mzasso/git/nodejs/node/out/Release/obj/gen/src/inspector" --config inspector_protocol_config.json
rm 9349840c76d301fd95cb4ac8c5f455128549de34.intermediate
if [ ! -r node -o ! -L node ]; then ln -fs out/Release/node node; fi
make build-addons
make -C out BUILDTYPE=Release V=1
  touch 9349840c76d301fd95cb4ac8c5f455128549de34.intermediate
  LD_LIBRARY_PATH=/home/mzasso/git/nodejs/node/out/Release/lib.host:/home/mzasso/git/nodejs/node/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../deps/v8/src/inspector; mkdir -p /home/mzasso/git/nodejs/node/out/Release/obj/gen/src/inspector/protocol /home/mzasso/git/nodejs/node/out/Release/obj/gen/include/inspector; python ../../third_party/inspector_protocol/CodeGenerator.py --jinja_dir ../../third_party --output_base "/home/mzasso/git/nodejs/node/out/Release/obj/gen/src/inspector" --config inspector_protocol_config.json
rm 9349840c76d301fd95cb4ac8c5f455128549de34.intermediate
if [ ! -r node -o ! -L node ]; then ln -fs out/Release/node node; fi
rm -f -r test/addons/??_*/
./node tools/doc/addon-verify.js
/home/mzasso/git/nodejs/node/tools/doc/addon-verify.js:13
const tokens = marked.lexer(contents);
                     ^

TypeError: Cannot read property 'lexer' of undefined
    at Object.<anonymous> (/home/mzasso/git/nodejs/node/tools/doc/addon-verify.js:13:22)
    at Module._compile (module.js:598:30)
    at Object.Module._extensions..js (module.js:609:10)
    at Module.load (module.js:532:32)
    at tryModuleLoad (module.js:495:12)
    at Function.Module._load (module.js:489:3)
    at Function.Module.runMain (module.js:634:10)
    at startup (bootstrap_node.js:158:16)
    at bootstrap_node.js:575:3
Makefile:230: recipe for target 'test/addons/.docbuildstamp' failed
make[1]: *** [test/addons/.docbuildstamp] Error 1
Makefile:196: recipe for target 'test' failed
make: *** [test] Error 2
make test-parallel
$ make test-parallel
make -C out BUILDTYPE=Release V=1
  touch 9349840c76d301fd95cb4ac8c5f455128549de34.intermediate
  LD_LIBRARY_PATH=/home/mzasso/git/nodejs/node/out/Release/lib.host:/home/mzasso/git/nodejs/node/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../deps/v8/src/inspector; mkdir -p /home/mzasso/git/nodejs/node/out/Release/obj/gen/src/inspector/protocol /home/mzasso/git/nodejs/node/out/Release/obj/gen/include/inspector; python ../../third_party/inspector_protocol/CodeGenerator.py --jinja_dir ../../third_party --output_base "/home/mzasso/git/nodejs/node/out/Release/obj/gen/src/inspector" --config inspector_protocol_config.json
rm 9349840c76d301fd95cb4ac8c5f455128549de34.intermediate
if [ ! -r node -o ! -L node ]; then ln -fs out/Release/node node; fi
/usr/bin/python tools/test.py --mode=release parallel -J
=== release test-assert-checktag ===
Path: parallel/test-assert-checktag
/home/mzasso/git/nodejs/node/test/parallel/test-assert-checktag.js:17
  return common.expectsError({
               ^

TypeError: Cannot read property 'expectsError' of undefined
    at re (/home/mzasso/git/nodejs/node/test/parallel/test-assert-checktag.js:17:16)
    at Object.<anonymous> (/home/mzasso/git/nodejs/node/test/parallel/test-assert-checktag.js:39:19)
    at Module._compile (module.js:598:30)
    at Object.Module._extensions..js (module.js:609:10)
    at Module.load (module.js:532:32)
    at tryModuleLoad (module.js:495:12)
    at Function.Module._load (module.js:489:3)
    at Function.Module.runMain (module.js:634:10)
    at startup (bootstrap_node.js:158:16)
    at bootstrap_node.js:575:3
Command: out/Release/node /home/mzasso/git/nodejs/node/test/parallel/test-assert-checktag.js
=== release test-assert-deep ===
Path: parallel/test-assert-deep
/home/mzasso/git/nodejs/node/test/parallel/test-assert-deep.js:17
  return common.expectsError({
               ^

TypeError: Cannot read property 'expectsError' of undefined
    at re (/home/mzasso/git/nodejs/node/test/parallel/test-assert-deep.js:17:16)
    at Object.<anonymous> (/home/mzasso/git/nodejs/node/test/parallel/test-assert-deep.js:34:17)
    at Module._compile (module.js:598:30)
    at Object.Module._extensions..js (module.js:609:10)
    at Module.load (module.js:532:32)
    at tryModuleLoad (module.js:495:12)
    at Function.Module._load (module.js:489:3)
    at Function.Module.runMain (module.js:634:10)
    at startup (bootstrap_node.js:158:16)
    at bootstrap_node.js:575:3
Command: out/Release/node /home/mzasso/git/nodejs/node/test/parallel/test-assert-deep.js
=== release test-assert ===
Path: parallel/test-assert
/home/mzasso/git/nodejs/node/test/parallel/test-assert.js:42
  common.expectsError({
        ^

TypeError: Cannot read property 'expectsError' of undefined
    at Object.<anonymous> (/home/mzasso/git/nodejs/node/test/parallel/test-assert.js:42:9)
    at Module._compile (module.js:598:30)
    at Object.Module._extensions..js (module.js:609:10)
    at Module.load (module.js:532:32)
    at tryModuleLoad (module.js:495:12)
    at Function.Module._load (module.js:489:3)
    at Function.Module.runMain (module.js:634:10)
    at startup (bootstrap_node.js:158:16)
    at bootstrap_node.js:575:3
Command: out/Release/node /home/mzasso/git/nodejs/node/test/parallel/test-assert.js
=== release test-async-hooks-close-during-destroy ===
Path: parallel/test-async-hooks-close-during-destroy
/home/mzasso/git/nodejs/node/test/parallel/test-async-hooks-close-during-destroy.js:14
  init: common.mustCallAtLeast((id, provider, triggerAsyncId) => {
              ^

TypeError: Cannot read property 'mustCallAtLeast' of undefined
    at Object.<anonymous> (/home/mzasso/git/nodejs/node/test/parallel/test-async-hooks-close-during-destroy.js:14:15)
    at Module._compile (module.js:598:30)
    at Object.Module._extensions..js (module.js:609:10)
    at Module.load (module.js:532:32)
    at tryModuleLoad (module.js:495:12)
    at Function.Module._load (module.js:489:3)
    at Function.Module.runMain (module.js:634:10)
    at startup (bootstrap_node.js:158:16)
    at bootstrap_node.js:575:3
Command: out/Release/node /home/mzasso/git/nodejs/node/test/parallel/test-async-hooks-close-during-destroy.js

...

TryCatch compileCatch(iso);
auto maybe_mod = ScriptCompiler::CompileModule(iso, &source);
if (compileCatch.HasCaught()) {
compileCatch.ReThrow();
Copy link
Member

Choose a reason for hiding this comment

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

Not sure but I think all the TryCatch can be replaced by maybe.isEmpty() checks. /cc @addaleax

Copy link
Member

Choose a reason for hiding this comment

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

Yes, checks like these should just be maybe.IsEmpty()

(Unless there is some weird interaction with uncaught exception handlers that this code relies on, but I don’t think that is the case, and if it is, that should be documented)

@Fishrock123 Fishrock123 self-assigned this Jul 20, 2017
@Fishrock123
Copy link
Contributor

adding myself as an assignee in an attempt to not forget about it

@TimothyGu TimothyGu self-assigned this Jul 21, 2017
@TimothyGu
Copy link
Member

TimothyGu commented Jul 21, 2017

The entire test suite passes here after dbd1ec3550423b0476d57bf638594bdd41e5f607. @targos can you recheck?

@targos
Copy link
Member

targos commented Jul 21, 2017

@TimothyGu I pushed a last fix. Now make test passes.

@addaleax
Copy link
Member

@targos @TimothyGu You are awesome! 💙

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

Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Some rough review. Will give it a try to day hopefully.

I suppose we will need quite the assortment of test cases? Or are there already standard tests somewhere?

Also all errors should probably switched to the new internal/errors system.

const loader = new Loader();

exports = module.exports = {
loader,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file only necessary to keep a global loader instance?

In other modules e.g. the repl, the REPL object has it as a static reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

we could move it to be a static ref since these files are never exposed to user code.

src/node_url.cc Outdated
@@ -1283,7 +1283,9 @@ void URL::Parse(const char* input,
}
break;
case kNoScheme:
cannot_be_base = base->flags & URL_FLAGS_CANNOT_BE_BASE;
if (has_base) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be in a separate commit/pr?

Copy link
Member Author

Choose a reason for hiding this comment

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

dunno / after #14096 I think PRs are more intrisicly bundled if you need changes to land things, but might be mistaken

Copy link
Member

Choose a reason for hiding this comment

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

I'll split it out as it's a bug in a code path actually used by code in master right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

please do split it out

Copy link
Member

Choose a reason for hiding this comment

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

+1 to splitting it out. It may be worthwhile having a test case for it also since it doesn't seem to be caught by the WPT tests at all.

Copy link
Member

Choose a reason for hiding this comment

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

node.gyp Outdated
# headers to make for a more pleasant IDE experience
'src/loader/module_wrap.h',
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these should be alphabetical. lib/internal/bootstrap_node.js', is an exception for some historical reasons.

Copy link
Member

Choose a reason for hiding this comment

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

I’m also wondering, is there any particular reason for having this be in its own directory?

Copy link
Member Author

Choose a reason for hiding this comment

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

@addaleax was for my own organization but we can keep with all files in a single dir if you wish.

lib/module.js Outdated
@@ -32,6 +32,7 @@ const path = require('path');
const internalModuleReadFile = process.binding('fs').internalModuleReadFile;
const internalModuleStat = process.binding('fs').internalModuleStat;
const preserveSymlinks = !!process.binding('config').preserveSymlinks;
const ESM = require('internal/loader/index');
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldn't internal/loader be enough with an index.js?

Copy link
Member

Choose a reason for hiding this comment

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

The internal modules work differently, there is no path resolution mechanism for them

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right.

filename = Module._resolveFilename(request, parent, isMain);
} catch (e) {
// try to keep stack
e.stack;
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for?

Is this actually a getter?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

lib/module.js Outdated
throw e;
}
);
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this somehow set these, as below?

process.mainModule = module;
module.id = '.';

}
}
};
const makeSafe = (unsafe, safe) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be function () {}s?

(Please don't export arrow functions unless absolutely necessary.)

Copy link
Member

Choose a reason for hiding this comment

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

Do we have a policy around that? What’s the reason?

Copy link
Member

Choose a reason for hiding this comment

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

Why? Arrow functions aren't constructable, so you'd be exporting a tighter API

Copy link
Member Author

Choose a reason for hiding this comment

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

@Fishrock123 can you clarify why?

Copy link
Member

Choose a reason for hiding this comment

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

We haven't had much of a defined policy about this. There have been cases where we have avoided changing existing exports from regular functions to arrow functions but given that this is a new API, I'm not sure if there's an issue here.

try {
return pathToFileURL(process.cwd());
} catch (e) {
// If the current working directory no longer exists.
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like the regular modular loader may explode in this case?

We should keep the behavior consistent whatever it is.

Copy link
Member

Choose a reason for hiding this comment

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

The regular loader doesn't explode. There are actually test cases for that. Preserving the original behavior is what I'm trying to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

seems sane

class Loader {
constructor(base = getBase()) {
this.moduleMap = new ModuleMap();
this.base = base ? new URL(`${base}`) : base;
Copy link
Contributor

Choose a reason for hiding this comment

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

should this throw or something if base is undefined or null?

Copy link
Member

Choose a reason for hiding this comment

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

See above, this is necessary to keep Loader construction from throwing, in a case when it's still perfectly fine to use native modules and absolute paths.

@targos
Copy link
Member

targos commented Jul 21, 2017

@TimothyGu
Copy link
Member

@Fishrock123 There are some test cases in @bmeck's sketchy-as-heck repo, I assume he has not yet had the time to port them over.

@TimothyGu
Copy link
Member

TBH I'm still not sure about the whole safe_globals business. If someone can explain its necessity that would be best.

@addaleax
Copy link
Member

addaleax commented Jul 21, 2017

New CI: https://ci.nodejs.org/job/node-test-commit/11293/ (edit: green)

@TimothyGu TimothyGu added module Issues and PRs related to the module subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 23, 2017
@TimothyGu
Copy link
Member

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

A recap of my work done:

  • CJS now works fully with module loading
  • Some basic .mjs tests were imported from sketchy, but I'm not yet sure if I want to import the ones using the internal Loader class yet.
  • Linter now supports .mjs too
  • process.exit(1) instead of emitting the annoying rejected promise w/o handler warning and exiting with code 1; this can be done because the ESM loader is only used for the main script right now.

Pending the resolution from nodejs/node-eps#60, maybe we should delay the work on this a bit before the resolver algorithm is set in stone. Some good arguments from both sides.

return getNamespaceOf(module);
}
}
Object.setPrototypeOf(Loader.prototype, Object.create(null));

Choose a reason for hiding this comment

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

What is the reason for this line? I am curious about its effect on performance.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an encapsulation general thing so that it does not inherit from Object so cannot be affected by mutated globals. In line with #10135

@bmeck
Copy link
Member Author

bmeck commented Jul 25, 2017

@TimothyGu ( #14369 (comment) )

Pending the resolution from nodejs/node-eps#60, maybe we should delay the work on this a bit before the resolver algorithm is set in stone. Some good arguments from both sides.

That PR is additive and does not remove or conflict with any features of this PR. There is no need to delay this PR unless you have concerns with conflict either now or in the future.

MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
This follows the EPS an allows the node CLI to have ESM as an entry point.
`node ./example.mjs`. A newer V8 is needed for `import()` so that is not
included. `import.meta` is still in specification stage so that also is not
included.

PR-URL: #14369
Author: Bradley Farias <[email protected]>
Author: Guy Bedford <[email protected]>
Author: Jan Krems <[email protected]>
Author: Timothy Gu <[email protected]>
Author: Michaël Zasso <[email protected]>
Author: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
This follows the EPS an allows the node CLI to have ESM as an entry point.
`node ./example.mjs`. A newer V8 is needed for `import()` so that is not
included. `import.meta` is still in specification stage so that also is not
included.

PR-URL: #14369
Author: Bradley Farias <[email protected]>
Author: Guy Bedford <[email protected]>
Author: Jan Krems <[email protected]>
Author: Timothy Gu <[email protected]>
Author: Michaël Zasso <[email protected]>
Author: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
This follows the EPS an allows the node CLI to have ESM as an entry point.
`node ./example.mjs`. A newer V8 is needed for `import()` so that is not
included. `import.meta` is still in specification stage so that also is not
included.

PR-URL: #14369
Author: Bradley Farias <[email protected]>
Author: Guy Bedford <[email protected]>
Author: Jan Krems <[email protected]>
Author: Timothy Gu <[email protected]>
Author: Michaël Zasso <[email protected]>
Author: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
tniessen added a commit that referenced this pull request Sep 12, 2017
PR-URL: #15290
Refs: #14369
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins added a commit that referenced this pull request Sep 12, 2017
Notable Changes

* build:
  * Snapshots are now re-enabled in V8
  #14875

* console:
  * Implement minimal `console.group()`.
  #14910

* deps:
  * upgrade libuv to 1.14.1
    #14866
  * update nghttp2 to v1.25.0
    #14955

* dns:
  * Add `verbatim` option to dns.lookup(). When true, results from the
    DNS resolver are passed on as-is, without the reshuffling that
    Node.js otherwise does that puts IPv4 addresses before IPv6
    addresses.
    #14731

* fs:
  * add fs.copyFile and fs.copyFileSync which allows for more efficient
    copying of files.
    #15034

* inspector:
  * Enable async stack traces
    #13870

* module:
  * Add support for ESM. This is currently behind the
    `--experimental-modules` flag and requires the .mjs extension.
    `node --experimental-modules index.mjs`
    #14369

* napi:
  * implement promise
    #14365

* os:
  * Add support for CIDR notation to the output of the
    networkInterfaces() method.
    #14307

* perf_hooks:
  * An initial implementation of the Performance Timing API for
    Node.js. This is the same Performance Timing API implemented by
    modern browsers with a number of Node.js specific properties. The
    User Timing mark() and measure() APIs are implemented, as is a
    Node.js specific flavor of the Frame Timing for measuring event
    loop duration.
    #14680

* tls:
  * multiple PFX in createSecureContext
    [#14793](#14793)

* Added new collaborators:
  * BridgeAR – Ruben Bridgewater

PR-URL: #15308
MylesBorins added a commit that referenced this pull request Sep 12, 2017
Notable Changes

* build:
  * Snapshots are now re-enabled in V8
  #14875

* console:
  * Implement minimal `console.group()`.
  #14910

* deps:
  * upgrade libuv to 1.14.1
    #14866
  * update nghttp2 to v1.25.0
    #14955

* dns:
  * Add `verbatim` option to dns.lookup(). When true, results from the
    DNS resolver are passed on as-is, without the reshuffling that
    Node.js otherwise does that puts IPv4 addresses before IPv6
    addresses.
    #14731

* fs:
  * add fs.copyFile and fs.copyFileSync which allows for more efficient
    copying of files.
    #15034

* inspector:
  * Enable async stack traces
    #13870

* module:
  * Add support for ESM. This is currently behind the
    `--experimental-modules` flag and requires the .mjs extension.
    `node --experimental-modules index.mjs`
    #14369

* napi:
  * implement promise
    #14365

* os:
  * Add support for CIDR notation to the output of the
    networkInterfaces() method.
    #14307

* perf_hooks:
  * An initial implementation of the Performance Timing API for
    Node.js. This is the same Performance Timing API implemented by
    modern browsers with a number of Node.js specific properties. The
    User Timing mark() and measure() APIs are implemented, as is a
    Node.js specific flavor of the Frame Timing for measuring event
    loop duration.
    #14680

* tls:
  * multiple PFX in createSecureContext
    [#14793](#14793)

* Added new collaborators:
  * BridgeAR – Ruben Bridgewater

PR-URL: #15308
@styfle
Copy link
Member

styfle commented Sep 13, 2017

This is super exciting, thanks for all your hard work! 🎉

I read the Interop with existing modules part of the docs and noticed CommonJS modules are exported as default which makes sense since there were no named exports in CommonJS.

This means I can use the following syntax just fine:

import fs from 'fs';
import _ from 'lodash';

But I cannot use the following syntax:

import * as fs from 'fs';
import * as _ from 'lodash';

Most importantly, I cannot use the following syntax:

import { copyFile } from 'fs';
import { map } from 'lodash';

For node core APIs that's probably okay. But for all the existing CommonJS modules on npm, this could be a problem because tools that provide tree-shaking, expect the latter syntax.

Is there a plan to support the latter syntax for importing smaller pieces of CommonJS modules?

@rodoabad
Copy link

You're the man @bmeck GG!

@bmeck
Copy link
Member Author

bmeck commented Sep 13, 2017

@styfle

But I cannot use the following syntax:

import * as fs from 'fs';
import * as _ from 'lodash';

Most importantly, I cannot use the following syntax:

import { copyFile } from 'fs';
import { map } from 'lodash';

The syntax by which things are imported does not change the ModuleNamespace objects that are created in the ES spec (the result of * as fs).

We have a few problems supporting namespace creation for CJS with custom names for a few reasons and are trying to figure out an opt-in solution that module authors could do. In particular, we need to know the list of property names that module.exports contains at parse time. We cannot do this with static analysis and will require authors to list these properties. Another problem is mutation of these properties cannot be safely observed and tracked for "live binding" that ES modules do. There will need to be invariant checks that the property descriptors are not configurable/writable/containing getters when module evalutation finishes, similar to Proxy invariant checks.

As for tree shaking, it should still work the same as tools using that do not tree shake CJS modules (except https://github.com/indutny/webpack-common-shake, which should be unaffected). In real ESM there is no tree shaking so that is not an issue.

Named properties are not on the top of my TODO list since they require opt-in changes and can not act like what babel does, but if someone wants to help spec out a pragma or something for opt-in; I am willing to review/help.

addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
This follows the EPS an allows the node CLI to have ESM as an entry point.
`node ./example.mjs`. A newer V8 is needed for `import()` so that is not
included. `import.meta` is still in specification stage so that also is not
included.

PR-URL: nodejs#14369
Author: Bradley Farias <[email protected]>
Author: Guy Bedford <[email protected]>
Author: Jan Krems <[email protected]>
Author: Timothy Gu <[email protected]>
Author: Michaël Zasso <[email protected]>
Author: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
PR-URL: nodejs#15290
Refs: nodejs#14369
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
addaleax pushed a commit to addaleax/node that referenced this pull request Sep 13, 2017
Notable Changes

* build:
  * Snapshots are now re-enabled in V8
  nodejs#14875

* console:
  * Implement minimal `console.group()`.
  nodejs#14910

* deps:
  * upgrade libuv to 1.14.1
    nodejs#14866
  * update nghttp2 to v1.25.0
    nodejs#14955

* dns:
  * Add `verbatim` option to dns.lookup(). When true, results from the
    DNS resolver are passed on as-is, without the reshuffling that
    Node.js otherwise does that puts IPv4 addresses before IPv6
    addresses.
    nodejs#14731

* fs:
  * add fs.copyFile and fs.copyFileSync which allows for more efficient
    copying of files.
    nodejs#15034

* inspector:
  * Enable async stack traces
    nodejs#13870

* module:
  * Add support for ESM. This is currently behind the
    `--experimental-modules` flag and requires the .mjs extension.
    `node --experimental-modules index.mjs`
    nodejs#14369

* napi:
  * implement promise
    nodejs#14365

* os:
  * Add support for CIDR notation to the output of the
    networkInterfaces() method.
    nodejs#14307

* perf_hooks:
  * An initial implementation of the Performance Timing API for
    Node.js. This is the same Performance Timing API implemented by
    modern browsers with a number of Node.js specific properties. The
    User Timing mark() and measure() APIs are implemented, as is a
    Node.js specific flavor of the Frame Timing for measuring event
    loop duration.
    nodejs#14680

* tls:
  * multiple PFX in createSecureContext
    [nodejs#14793](nodejs#14793)

* Added new collaborators:
  * BridgeAR – Ruben Bridgewater

PR-URL: nodejs#15308
@jdalton
Copy link
Member

jdalton commented Sep 13, 2017

@bmeck

Named properties are not on the top of my TODO list since they require opt-in changes and can not act like what babel does, but if someone wants to help spec out a pragma or something for opt-in; I am willing to review/help.

From the July 26 TC39 notes you said:

Node is not going to diverge from the browser on this topic. If "use module" is not supported by browsers, Node is not going to use "use module" either.

Are you now open to Node specific pragmas or does this mean you'd want the CJS import named exports pragma to make its way to the TC39? Is it too Node specific?

@bmeck
Copy link
Member Author

bmeck commented Sep 14, 2017

@jdalton Node specific things within Node only CJS things (module.exports) seems fine, there is no compatibility concerns since other environments would not be affected. I would be very against it being applied to all things/files that might rely on the pragma but are intended to work without Node. Also, this idea is adding invariant checks rather than changing runtime/compiletime behavior.

jasnell pushed a commit that referenced this pull request Sep 20, 2017
PR-URL: #15290
Refs: #14369
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
}

async resolve(specifier) {
const request = resolveRequestUrl(this.base, specifier);
Copy link

Choose a reason for hiding this comment

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

Missing await statement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. module Issues and PRs related to the module subsystem. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.