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

esm resolver spec and implementation refinements #12

Closed
wants to merge 3 commits into from

Conversation

guybedford
Copy link
Contributor

@guybedford guybedford commented Oct 24, 2018

Moved from original posting at nodejs/modules#207:

In line with the current plans for Phase 2 I've put together a draft resolver algorithm here that includes the new properties from the minimal kernel implementation.

Having an easy-to-read algorithm will allow tools and bundlers to follow the resolver implementation, just like with the CommonJS resolver.

The goal is, like the minimal kernel, to start with what we have agreed on, and to expand this over time as we can reach consensus.

This one brings up the discussions around the package.json main handling.

Would be nice to discuss this at the meeting today if there is time, although I understand it is late notice to read up on the details here.

Readable link: https://github.com/nodejs/ecmascript-modules/blob/esm-resolver-spec/doc/api/esm.md#resolution-algorithm

doc/api/esm.md Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

* Relative and absolute URL resolution
* No default extensions
* No directory lookups
* Bare specifier package resolution lookup through node_modules
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should take on the historical burden of node_modules in the default implementation. It's inefficient, hard to predict (e.g. it might load random things from a user's home directory), and enshrines a directory layout that many people (myself included) are trying to get away from.

Should we maybe split this into scoped resolution and an example for how the underlying data for scoped resolution might be calculated from a directory tree?

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 confused, how is it hard to predict?

That directory layout isn't something that can be trivially nor any time soon gotten away from.

@devsnek
Copy link
Member

devsnek commented Oct 24, 2018

for the purposes of readability would it be possible to

  • wrap all the specifiers in quotes (preferably double)
  • change _FN(xyz)_ to _FN(_xyz_)_ so the arguments aren't italicized and stand out more.

@guybedford
Copy link
Contributor Author

@devsnek thanks, always open to spec style suggestions, I've added those.

doc/api/esm.md Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
bmeck
bmeck previously requested changes Nov 1, 2018
doc/api/esm.md Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
@guybedford
Copy link
Contributor Author

@bmeck thanks for taking a look - all feedback has been updated in the spec.

@bmeck bmeck dismissed their stale review November 1, 2018 15:39

seems like my concerns have been addressed, still thinking on things and need to reread probably

@guybedford guybedford changed the title doc: Draft esm resolver spec doc: esm resolver spec Nov 2, 2018
@guybedford
Copy link
Contributor Author

@nodejs/modules Initially I attempted to remove directory indexes from this PR entirely, as in the current minimal implementation, but this again results in the friction that import 'pkg' will apply the main but import '/path/to/node_modules/pkg' won't apply the main which just seems like it will break a lot of things having this inconsistency.

The same applies for pkg/x where x is a directory with a main. Where we then have the issue that if /path/to/node_modules/pkg/x will work, then we should have pkg/x also work here for consistency.

Note that no extra statting is incurred by this, and it gives us maximum backwards compatibility.

But what it comes down to is that folder mains almost impossible to deprecate, unless we want to break lots of things.

doc/api/esm.md Outdated Show resolved Hide resolved
@guybedford guybedford changed the title doc: esm resolver spec esm resolver spec and implementation refinements Nov 6, 2018
@guybedford
Copy link
Contributor Author

guybedford commented Nov 6, 2018

I've gone ahead and updated this PR with an implementation of the spec as written as well.

In the process the error handling has also been improved to include the parent importer path when throwing not found errors, as well as providing the most relevant version of the specifier in the resolution process.

For example if resolving import 'x/y' where the package 'x' exists at node_modules/x but without a y, the error thrown will read:

Cannot find module '/path/to/node_modules/x/y' imported from /path/to/x.mjs

This is also a consequence of no longer implementing the node_modules fallback for missing subpaths.

In addition error message consistency is provided to ensure all NOT FOUND errors have the same initial description structure of Cannot find module '%s' imported from %s.

Apart from that the rest just implements the spec directly.

} else {
// TODO(bmeck) PREVENT FALLTHROUGH
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note this TODO is fixed :)

@guybedford
Copy link
Contributor Author

In the name of a minimal implementation I've made the following further changes here:

  1. Remove folder mains - these can always be added back
  2. Removed the index.mjs main to simply have no main currently - rather splitting this out as further work.
  3. Unified error handling to have all resolver errors be ERR_MODULE_NOT_FOUND instead of having ERR_MODULE_LEGACY, ERR_MISSING_MODULE, MODULE_NOT_FOUND all meaning the same thing.

@jkrems
Copy link
Contributor

jkrems commented Nov 7, 2018

+1 to merging this to allow further iteration.

@guybedford
Copy link
Contributor Author

Further to the discussion in today's meeting I've removed all main handling and parsing from this PR.

@nodejs/modules I would be very grateful if we can merge this before the next meeting so that we can consider PRs against this then. If everyone could take a moment to review I'd be very grateful to get this spec in!

@guybedford
Copy link
Contributor Author

@giltayar hopefully this allays your concerns here?

Copy link
Contributor

@giltayar giltayar left a comment

Choose a reason for hiding this comment

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

Just to make sure: this spec (currently) allows as imports foo/x.mjs, but not foo?

If this is correct, then LGTM.

doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Outdated Show resolved Hide resolved
doc/api/esm.md Show resolved Hide resolved
@MylesBorins MylesBorins restored the esm-resolver-spec branch March 13, 2019 21:40
MylesBorins added a commit that referenced this pull request Mar 14, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 14, 2019
Refs: nodejs/modules#180

PR-URL: #6
PR-URL: #12
Co-authored-by: Myles Borins <[email protected]>
Co-authored-by: John-David Dalton <[email protected]>
MylesBorins added a commit that referenced this pull request Mar 15, 2019
MylesBorins added a commit that referenced this pull request Mar 18, 2019
MylesBorins added a commit that referenced this pull request Mar 18, 2019
MylesBorins added a commit that referenced this pull request Mar 18, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 19, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 20, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 21, 2019
MylesBorins added a commit that referenced this pull request Mar 21, 2019
MylesBorins added a commit that referenced this pull request Mar 21, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 22, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 23, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 24, 2019
MylesBorins added a commit that referenced this pull request Mar 26, 2019
nodejs-ci pushed a commit that referenced this pull request Mar 27, 2019
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Mar 27, 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.

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: WICG/webcomponents#770
Co-authored-by: Myles Borins <[email protected]>
Co-authored-by: John-David Dalton <[email protected]>
Co-authored-by: Evan Plaice <[email protected]>
Co-authored-by: Geoffrey Booth <[email protected]>
Co-authored-by: Michaël Zasso <[email protected]>

PR-URL: nodejs#26745
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Apr 5, 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.

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: WICG/webcomponents#770
Co-authored-by: Myles Borins <[email protected]>
Co-authored-by: John-David Dalton <[email protected]>
Co-authored-by: Evan Plaice <[email protected]>
Co-authored-by: Geoffrey Booth <[email protected]>
Co-authored-by: Michaël Zasso <[email protected]>

PR-URL: #26745
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.