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

new ESM implementation #26745

Merged
merged 1 commit into from Mar 27, 2019

Conversation

@MylesBorins
Copy link
Member

MylesBorins commented Mar 18, 2019

This PR updates the current --experimental-modules implementation based on the work of the modules team and reflects Phase 2 of our new modules plan.

A longer form description of these changes can be found in our draft blog post.

The largest differences from the current implementation include

  • packge.type which can be either module or commonjs
    • type: "commonjs":
      • .js is parsed as commonjs
      • default for entry point without an extension is commonjs
    • type: "module":
      • .js is parsed as esm
      • does not support loading JSON or Native Module by default
      • default for entry point without an extension is esm
  • --type=[mode] to let you set the type on entry point. Will override package.type for entry point.
  • A new file extension .cjs.
    • this is specifically to support importing commonjs in the module mode.
    • this is only in the esm loader, the commonjs loader remains untouched, but the extension will work in the old loader if you use the full file path.
  • --es-module-specifier-resolution=[type]
    • options are explicit (default) and node
    • by default our loader will not allow for optional extensions in the import, the path for a module must include the extension if there is one
    • by default our loader will not allow for importing directories that have an index file
    • developers can use --es-module-specifier-resolution=node to enable the commonjs specifier resolution algorithm
    • This is not a “feature” but rather an implementation for experimentation. It is expected to change before the flag is removed
  • --experimental-json-loader
    • the only way to import json when "type": "module"
    • when enable all import 'thing.json' will go through the experimental loader independent of mode
    • based on whatwg/html#4315
  • You can use package.main to set an entry point for a module
    • the file extensions used in main will be resolved based on the type of the module
@MylesBorins

This comment has been minimized.

Copy link
Member Author

MylesBorins commented Mar 18, 2019

@MylesBorins MylesBorins requested review from guybedford and bmeck Mar 18, 2019

Show resolved Hide resolved lib/internal/errors.js Outdated
@devsnek
Copy link
Member

devsnek left a comment

I don't think the -m is a wise addition, being the only short flag that doesn't match the long one, and being the only short flag that can disagree with its long counterpart.

@antsmartian

This comment has been minimized.

Copy link
Contributor

antsmartian commented Mar 18, 2019

I was trying to build this locally on my mac machine to play around, but got back the following error:

/Users/anto/programs/node/node/out/Release/obj.target/v8_snapshot/geni/embedded.cc:5:10: fatal error: 'src/snapshot/macros.h' file not found
#include "src/snapshot/macros.h"
         ^~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[1]: *** [/Users/abelginrayen/programs/node/node/out/Release/obj.target/v8_snapshot/geni/embedded.o] Error 1
make[1]: *** Waiting for unfinished jobs....
rm f821e0c8fdac2538c3eb950594d8386c58c50c3d.intermediate b79cbb721bec5e5c6395b336bec4afcbcee7dbe9.intermediate
make: *** [node] Error 2

BTW, I'm on mac os sierra 10.12.6.

@MylesBorins

This comment has been minimized.

Copy link
Member Author

MylesBorins commented Mar 18, 2019

@antsmartian have you built node since the last V8 update? I had the same error as well and had to make clean and attempt to build again. This fixed it for me, (the issue was cache related)

@kristoferjoseph

This comment has been minimized.

Copy link

kristoferjoseph commented Mar 18, 2019

This PR doesn't mention .mjs what is the status of support for this extension?

@MylesBorins

This comment has been minimized.

Copy link
Member Author

MylesBorins commented Mar 18, 2019

@kristoferjoseph .mjs is unchanged. In the commonjs mode (which is deafult) .mjs is the only way to import esm. In the module mode .mjs can still be used.

@MylesBorins

This comment has been minimized.

Copy link
Member Author

MylesBorins commented Mar 18, 2019

@devsnek I've removed -m if you could remove your objection. We can bring the flag back to the modules team to review.

Show resolved Hide resolved doc/api/esm.md Outdated

@devsnek devsnek requested a review from vsemozhetbyt Mar 18, 2019

Show resolved Hide resolved doc/api/cli.md Outdated
Show resolved Hide resolved doc/api/cli.md Outdated
Show resolved Hide resolved doc/api/esm.md Outdated
Show resolved Hide resolved lib/internal/errors.js Outdated
Show resolved Hide resolved doc/api/cli.md Outdated
Show resolved Hide resolved doc/api/esm.md Outdated
Show resolved Hide resolved doc/api/esm.md Outdated

@MylesBorins MylesBorins force-pushed the MylesBorins:modules-lkgr branch from 298af1f to 3a40599 Mar 27, 2019

@MylesBorins

This comment has been minimized.

Copy link
Member Author

MylesBorins commented Mar 27, 2019

@nodejs-github-bot

This comment has been minimized.

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Mar 27, 2019

image

Green green green! Go go go!

@nodejs-github-bot

This comment has been minimized.

@BridgeAR
Copy link
Member

BridgeAR left a comment

Great work! This looks really good!

}
Module._load(process.argv[1], null, true);

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Mar 27, 2019

Member

Nit: this looks like an unrelated change.

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Mar 27, 2019

Author Member

@guybedford can you comment on this?

This comment has been minimized.

Copy link
@guybedford

guybedford Mar 27, 2019

Contributor

It's a very minor refactoring, without changing any logic at all. We can switch it back sure, but I prefer this form to be honest.

Show resolved Hide resolved lib/internal/modules/esm/default_resolve.js
Show resolved Hide resolved doc/node.1
Show resolved Hide resolved lib/internal/modules/esm/translators.js
Show resolved Hide resolved lib/internal/process/execution.js
Show resolved Hide resolved src/module_wrap.cc
@@ -2250,6 +2255,17 @@ size.
This `Error` is thrown when a read is attempted on a TTY `WriteStream`,
such as `process.stdout.on('data')`.

<a id="ERR_TYPE_MISMATCH"></a>
#### ERR_TYPE_MISMATCH

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Mar 27, 2019

Member

Since we renamed type to entry-type should this also be renamed accordingly? This seems very broad currently even though it's a pretty specific error.

This comment has been minimized.

Copy link
@GeoffreyBooth

GeoffreyBooth Mar 27, 2019

Contributor

It’s not just a mismatch of --entry-type and extension, this also covers a mismatch of --entry-type and package.json "type" field. So it is pretty broad. You could argue that both cases involve --entry-type, but naming it ERR_ENTRY_TYPE_MISMATCH implies that the problem is on the --entry-type side when really it could be on either side. The error message itself specifies what mismatches what.

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Mar 27, 2019

Author Member

I've renamed it entry type already and force pushed... LOL. We can discuss one last time in today's module meeting... if we need to change it one more time I can. Either way this can easily be updated while we are flagged

@MylesBorins MylesBorins force-pushed the MylesBorins:modules-lkgr branch 4 times, most recently from 3aa99f1 to 410c898 Mar 27, 2019

Show resolved Hide resolved doc/node.1 Outdated

@MylesBorins MylesBorins force-pushed the MylesBorins:modules-lkgr branch from 410c898 to d9cdfd8 Mar 27, 2019

@MylesBorins

This comment has been minimized.

Copy link
Member Author

MylesBorins commented Mar 27, 2019

one last CI: https://ci.nodejs.org/job/node-test-pull-request/21969/

The modules team is meeting in 5 minutes. Assuming there are no last minute changes I plan to land this implementation at the end of the meeting

@nodejs-github-bot

This comment has been minimized.

@ChALkeR
Copy link
Member

ChALkeR left a comment

Ok, rubber stamp LGTM.
I have not reviewed all the code, though, but the main changes here definitely look good.

Set the top-level module resolution type.
.
.It Fl -es-module-specifier-resolution
Select extension resolution algorithm for ES Modules; either 'explicit' (default) or 'node'

This comment has been minimized.

Copy link
@vsemozhetbyt

vsemozhetbyt Mar 27, 2019

Member

Sorry, still one nit (the period at the end hidden by the scrolling).

Suggested change
Select extension resolution algorithm for ES Modules; either 'explicit' (default) or 'node'
Select extension resolution algorithm for ES Modules; either 'explicit' (default) or 'node'.
esm: phase two of new esm implementation
This PR updates the current `--experimental-modules` implementation
based on the work of the modules team  and reflects Phase 2 of our
new modules plan.

The largest differences from the current implementation include

* `packge.type` which can be either `module` or `commonjs`
  - `type: "commonjs"`:
    - `.js` is parsed as commonjs
    - default for entry point without an extension is commonjs
  - `type: "module"`:
    - `.js` is parsed as esm
    - does not support loading JSON or Native Module by default
    - default for entry point without an extension is esm
* `--entry-type=[mode]`
  - allows you set the type on entry point.
* A new file extension `.cjs`.
  - this is specifically to support importing commonjs in the
    `module` mode.
  - this is only in the esm loader, the commonjs loader remains
    untouched, but the extension will work in the old loader if you use
    the full file path.
* `--es-module-specifier-resolution=[type]`
  - options are `explicit` (default) and `node`
  - by default our loader will not allow for optional extensions in
    the import, the path for a module must include the extension if
    there is one
  - by default our loader will not allow for importing directories that
    have an index file
  - developers can use `--es-module-specifier-resolution=node` to
    enable the commonjs specifier resolution algorithm
  - This is not a “feature” but rather an implementation for
    experimentation. It is expected to change before the flag is
    removed
* `--experimental-json-loader`
  - the only way to import json when `"type": "module"`
  - when enable all `import 'thing.json'` will go through the
    experimental loader independent of mode
  - based on whatwg/html#4315
* You can use `package.main` to set an entry point for a module
  - the file extensions used in main will be resolved based on the
    `type` of the module

Refs: https://github.com/nodejs/modules/blob/master/doc/plan-for-new-modules-implementation.md
Refs: https://github.com/GeoffreyBooth/node-import-file-specifier-resolution-proposal
Refs: nodejs/modules#180
Refs: nodejs/ecmascript-modules#6
Refs: nodejs/ecmascript-modules#12
Refs: nodejs/ecmascript-modules#28
Refs: nodejs/modules#255
Refs: whatwg/html#4315
Refs: w3c/webcomponents#770
Co-authored-by: Myles Borins <MylesBorins@google.com>
Co-authored-by: John-David Dalton <john.david.dalton@gmail.com>
Co-authored-by: Evan Plaice <evanplaice@gmail.com>
Co-authored-by: Geoffrey Booth <webmaster@geoffreybooth.com>
Co-authored-by: Michaël Zasso <targos@protonmail.com>

PR-URL: #26745
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

@MylesBorins MylesBorins force-pushed the MylesBorins:modules-lkgr branch from d9cdfd8 to b1094db Mar 27, 2019

@MylesBorins MylesBorins merged commit b1094db into nodejs:master Mar 27, 2019

BethGriggs added a commit that referenced this pull request Apr 5, 2019

esm: phase two of new esm implementation
This PR updates the current `--experimental-modules` implementation
based on the work of the modules team  and reflects Phase 2 of our
new modules plan.

The largest differences from the current implementation include

* `packge.type` which can be either `module` or `commonjs`
  - `type: "commonjs"`:
    - `.js` is parsed as commonjs
    - default for entry point without an extension is commonjs
  - `type: "module"`:
    - `.js` is parsed as esm
    - does not support loading JSON or Native Module by default
    - default for entry point without an extension is esm
* `--entry-type=[mode]`
  - allows you set the type on entry point.
* A new file extension `.cjs`.
  - this is specifically to support importing commonjs in the
    `module` mode.
  - this is only in the esm loader, the commonjs loader remains
    untouched, but the extension will work in the old loader if you use
    the full file path.
* `--es-module-specifier-resolution=[type]`
  - options are `explicit` (default) and `node`
  - by default our loader will not allow for optional extensions in
    the import, the path for a module must include the extension if
    there is one
  - by default our loader will not allow for importing directories that
    have an index file
  - developers can use `--es-module-specifier-resolution=node` to
    enable the commonjs specifier resolution algorithm
  - This is not a “feature” but rather an implementation for
    experimentation. It is expected to change before the flag is
    removed
* `--experimental-json-loader`
  - the only way to import json when `"type": "module"`
  - when enable all `import 'thing.json'` will go through the
    experimental loader independent of mode
  - based on whatwg/html#4315
* You can use `package.main` to set an entry point for a module
  - the file extensions used in main will be resolved based on the
    `type` of the module

Refs: https://github.com/nodejs/modules/blob/master/doc/plan-for-new-modules-implementation.md
Refs: https://github.com/GeoffreyBooth/node-import-file-specifier-resolution-proposal
Refs: nodejs/modules#180
Refs: nodejs/ecmascript-modules#6
Refs: nodejs/ecmascript-modules#12
Refs: nodejs/ecmascript-modules#28
Refs: nodejs/modules#255
Refs: whatwg/html#4315
Refs: w3c/webcomponents#770
Co-authored-by: Myles Borins <MylesBorins@google.com>
Co-authored-by: John-David Dalton <john.david.dalton@gmail.com>
Co-authored-by: Evan Plaice <evanplaice@gmail.com>
Co-authored-by: Geoffrey Booth <webmaster@geoffreybooth.com>
Co-authored-by: Michaël Zasso <targos@protonmail.com>

PR-URL: #26745
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@jkrems

This comment has been minimized.

Copy link
Contributor

jkrems commented Apr 5, 2019

@lukaszewczak brought up that this arguably should be included in #26930 but it looks like this just missed the cut of v12.x-staging?

@GeoffreyBooth

This comment has been minimized.

Copy link
Contributor

GeoffreyBooth commented Apr 11, 2019

Node core folks, please see #27184. It’s a follow-up to this PR that I’d love to see merged in before Node 12 ships 🙏

@MylesBorins

This comment has been minimized.

Copy link
Member Author

MylesBorins commented Apr 16, 2019

Adding dont-land-on-11.x... I think we should leave 11.x alone for the final release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.