Skip to content
This repository was archived by the owner on Apr 16, 2020. It is now read-only.

New ESM Implementation #60

Closed
wants to merge 6 commits into from
Closed

New ESM Implementation #60

wants to merge 6 commits into from

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Mar 14, 2019

This is what I plan to upstream tomorrow.

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 JSON modules 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

Did I miss anything?

@aduh95
Copy link
Contributor

aduh95 commented Mar 14, 2019

this changes the default file extension solution, specifically what .js should be considered

I guess that also changes behavior for extension-less files, right?

@GeoffreyBooth
Copy link
Member

I would mention --type next to "type", and bump .cjs down. Also there’s no context for why .cjs is needed, so it seems a bit arbitrary. You could copy the text from the announcement post.

Other new things per the post are:

  • import no longer supports JSON (without the new flag) and no longer supports native modules.
  • package.main can point to an ES module package entry point; dual packages are not currently supported.

@evanplaice
Copy link

Did I miss anything?

Was there ever a consensus on handling hashbang entry points? Is there going to be a node-esm alias for convenience.

Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Approved FWIW :)

Should we link to the work-in-progress blog post as part of the PR to give background? I feel like bullets don't exactly give the entire process justice.

Perhaps also mention the --es-module-specifier-resolution flag is experimental while we determine what the default behaviour will be, as opposed to the flag itself being the feature.

@MylesBorins
Copy link
Contributor Author

updated ptal

@GeoffreyBooth
Copy link
Member

It still doesn’t mention that native modules are unsupported in import, and that dual packages are unsupported. Not sure those need to be mentioned. LGTM.

@targos targos force-pushed the modules-lkgr-rebase branch 2 times, most recently from 9434bfb to c0bf5b3 Compare March 14, 2019 21:55
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Mar 15, 2019

Have now incorporated all fixes from from Q/A

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

@MylesBorins
Copy link
Contributor Author

Updated copy for PR message. PTAL

@MylesBorins MylesBorins force-pushed the modules-lkgr-rebase branch from c0bf5b3 to 5efc4fb Compare March 15, 2019 23:23
@MylesBorins MylesBorins changed the base branch from master to modules-lkgr March 15, 2019 23:27
@MylesBorins MylesBorins changed the base branch from modules-lkgr to master March 15, 2019 23:28
Copy link
Member

@GeoffreyBooth GeoffreyBooth left a comment

Choose a reason for hiding this comment

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

I fixed some missing backticks and other very minor formatting. LGTM.

@guybedford guybedford self-requested a review March 16, 2019 00:10
@MylesBorins MylesBorins force-pushed the modules-lkgr-rebase branch 2 times, most recently from 268993e to cb0d7d1 Compare March 18, 2019 14:11
guybedford and others added 6 commits March 18, 2019 10:13
There are currently two supported values
"explicit" and "node"
With the new flag `--experimental-json-modules` it is now possible
to import .json files. It piggy backs on the current cjs loader
implementation, so it only exports a default. This is a bit of a
hack, and it should potentially have it's own loader, especially
if we change the cjs loader at all.

The behavior for .json in the cjs loader matches the current
planned behavior if json modules were to be standardized, specifically
that a .json module only exports a default.

Refs: nodejs/modules#255
Refs: whatwg/html#4315
Refs: WICG/webcomponents#770
Co-authored-by: Myles Borins <[email protected]>
Co-authored-by: Evan Plaice <[email protected]>
@MylesBorins MylesBorins force-pushed the modules-lkgr-rebase branch from cb0d7d1 to 484d1fb Compare March 18, 2019 14:13
@MylesBorins
Copy link
Contributor Author

I've finished incorporating all of @guybedford's suggested changes and rebasing the branch to be pristine. Each individual commit passes the test suite, so this is IMHO ready to upstream.

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

Once CI is green I'll be opening this PR, ETA 2 - 3 hours

@MylesBorins MylesBorins changed the title [WIP] New ESM Implementation New ESM Implementation Mar 18, 2019
@MylesBorins
Copy link
Contributor Author

landed in modules-lkgr in 969c63a...484d1fb

upstream PR incoming

@ljharb ljharb deleted the modules-lkgr-rebase branch March 18, 2019 15:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants