--es-module-specifier-resolution flag #48

MylesBorins · 2019-03-05T09:44:32Z

This is a super naive copy / paste of the resolution algorithm from upstream, slightly tweaked to work with the internals of the new loader. There is some duplication here, but I've kept it to a minimum.

@ljharb the flag right now is a boolean flag... either the flag is included or isn't. Happy to work on something better, e.g. a no-op flag for other mode or different input type for this flag... but this was enough to get things stared

If anyone wants to make changes to this please feel free to push to the branch. Hopefully this gets us 90% of the way there

thysultan · 2019-03-05T10:10:07Z

What does legacy_resolution entail?

For example if you have the following node_module pkg structure:

pkg/
    server/
       index.js
       package.json
    package.json
    index.js

Assuming both package.json in pkg and pkg/server point to their respective index.js, would the following:

import {h} from 'pkg'
import {render} from 'pkg/server'

Under legacy_resolution still resolve index.js through package.json.

That is can you still archive "pretty paths" like pkg/server instead of pkg/server/index.js by using package.json, or is all of this unrelated to this.

guybedford · 2019-03-05T10:50:54Z

I've rebased this to the CI fixes branch, and also added an adjustment to ensure the legacy resolution applies to package subpath resolution paths as well.

devsnek · 2019-03-05T14:07:35Z

@MylesBorins just for the sake of avoiding confusion, can this be named --searching-resolution or something? "legacy" already refers to a bunch of behaviour in the cjs loader that the one you copied here doesn't have.

ljharb · 2019-03-05T17:36:13Z

Also “legacy” implies it’s legacy, which at the moment it is certainly not, and we have no consensus that it will be. Please rename.

Re a boolean flag; that’s fine if it follows the convention that --no-x sets x to false, while -x sets it to true.

src/module_wrap.cc

src/node_options.cc

src/module_wrap.cc

MylesBorins · 2019-03-06T07:47:42Z

Updated based on all notes and added extensive tests PTAL

test/fixtures/es-module-specifiers/node_modules/explicit-main/package.json

src/node_options.cc

MylesBorins · 2019-03-06T20:49:58Z

test/es-module/test-esm-specifiers.mjs

@@ -14,7 +14,7 @@ assert.strictEqual(commonjs, 'commonjs');
 assert.strictEqual(module, 'module');
 assert.strictEqual(success, 'success');
 assert.strictEqual(explicit, 'esm');
-assert.strictEqual(implicit, 'cjs');
+assert.strictEqual(implicit, 'esm');


So with this change, from the prior loader, .mjs would be resolved before .js? would .cjs resolve before .mjs?

Looks good to me, as long as if dual-mode is possible we leave it undocumented.

MylesBorins · 2019-03-08T23:08:29Z

This has been open for 4 days and there are no more objections. If CI is green I plan to land this.

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

There are currently two supported values "none" and "node"

MylesBorins · 2019-03-09T06:30:04Z

landed in bec588f

@sapegin

BUGFIXES * [`27cccfbda`](npm/cli@27cccfb) [#223](npm/cli#223) vulns → vulnerabilities in npm audit output ([@sapegin](https://github.com/sapegin)) * [`d5e865eb7`](npm/cli@d5e865e) [#222](npm/cli#222) [#226](npm/cli#226) install, doctor: don't crash if registry unset ([@dmitrydvorkin](https://github.com/dmitrydvorkin), [@isaacs](https://github.com/isaacs)) * [`5b3890226`](npm/cli@5b38902) [#227](npm/cli#227) [npm.community#9167](https://npm.community/t/npm-err-cb-never-called-permission-denied/9167/5) Handle unhandledRejections, tell user what to do when encountering an `EACCES` error in the cache. ([@isaacs](https://github.com/isaacs)) DEPENDENCIES * [`77516df6e`](npm/cli@77516df) `licensee@7.0.3` ([@isaacs](https://github.com/isaacs)) * [`ceb993590`](npm/cli@ceb9935) `query-string@6.8.2` ([@isaacs](https://github.com/isaacs)) * [`4050b9189`](npm/cli@4050b91) `hosted-git-info@2.8.2` * [#46](npm/hosted-git-info#46) [#43](npm/hosted-git-info#43) [#47](npm/hosted-git-info#47) [#44](npm/hosted-git-info#44) Add support for GitLab subgroups ([@mterrel](https://github.com/mterrel), [@isaacs](https://github.com/isaacs), [@ybiquitous](https://github.com/ybiquitous)) * [`3b1d629`](npm/hosted-git-info@3b1d629) [#48](npm/hosted-git-info#48) fix http protocol using sshurl by default ([@fengmk2](https://github.com/fengmk2)) * [`5d4a8d7`](npm/hosted-git-info@5d4a8d7) ignore noCommittish on tarball url generation ([@isaacs](https://github.com/isaacs)) * [`1692435`](npm/hosted-git-info@1692435) use gist tarball url that works for anonymous gists ([@isaacs](https://github.com/isaacs)) * [`d5cf830`](npm/hosted-git-info@d5cf830) Do not allow invalid gist urls ([@isaacs](https://github.com/isaacs)) * [`e518222`](npm/hosted-git-info@e518222) Use LRU cache to prevent unbounded memory consumption ([@iarna](https://github.com/iarna)) PR-URL: nodejs/node#29023 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins changed the title ~~[WIP] Legacy Resolution Falg~~ [WIP] Legacy Resolution Flag Mar 5, 2019

MylesBorins force-pushed the resolution-flag branch from f94cc2c to b36bf08 Compare March 5, 2019 09:48

MylesBorins mentioned this pull request Mar 5, 2019

Steps required to upstream implementation nodejs/modules#286

Closed

12 tasks

guybedford force-pushed the resolution-flag branch from b36bf08 to 2168312 Compare March 5, 2019 10:49

MylesBorins force-pushed the modules-lkgr branch from 15daf0a to 03fb3b1 Compare March 5, 2019 18:43

MylesBorins changed the title ~~[WIP] Legacy Resolution Flag~~ [WIP] --esm-extension-resolution flag Mar 5, 2019

MylesBorins force-pushed the resolution-flag branch from 2de50e3 to d6a3b09 Compare March 5, 2019 19:49

This comment has been minimized.

Sign in to view

MylesBorins force-pushed the resolution-flag branch from d6a3b09 to 6d32f24 Compare March 5, 2019 20:07

MylesBorins changed the title ~~[WIP] --esm-extension-resolution flag~~ [WIP] --esm-filename-resolution flag Mar 5, 2019

GeoffreyBooth suggested changes Mar 6, 2019

View reviewed changes

src/module_wrap.cc Outdated Show resolved Hide resolved

src/module_wrap.cc Show resolved Hide resolved

src/node_options.cc Outdated Show resolved Hide resolved

MylesBorins added the enhancement New feature or request label Mar 6, 2019

GeoffreyBooth reviewed Mar 6, 2019

View reviewed changes

src/module_wrap.cc Outdated Show resolved Hide resolved

MylesBorins changed the title ~~[WIP] --esm-filename-resolution flag~~ [WIP] --es-module-specifier-resolution flag Mar 6, 2019

MylesBorins force-pushed the resolution-flag branch 4 times, most recently from a17b130 to 3e3bb27 Compare March 6, 2019 07:46

GeoffreyBooth previously requested changes Mar 6, 2019

View reviewed changes

test/fixtures/es-module-specifiers/node_modules/explicit-main/package.json Show resolved Hide resolved

src/node_options.cc Outdated Show resolved Hide resolved

nodejs-ci force-pushed the modules-lkgr branch from 6c793ca to bd5c877 Compare March 6, 2019 09:10

MylesBorins force-pushed the resolution-flag branch from 6864e68 to 7a3b64e Compare March 6, 2019 14:43

MylesBorins commented Mar 6, 2019

View reviewed changes

nodejs-ci force-pushed the modules-lkgr branch from bd5c877 to 0237465 Compare March 7, 2019 09:06

guybedford approved these changes Mar 7, 2019

View reviewed changes

MylesBorins force-pushed the resolution-flag branch from dcee790 to a7b4b27 Compare March 8, 2019 02:40

MylesBorins and others added 6 commits March 8, 2019 19:53

esm: add --es-module-specifier-resolution

fe296bf

There are currently two supported values "none" and "node"

fixup: missing tests

ae8566b

fixup: change default mode name

4831b2a

simplify package main resolve

dde193c

implicit specifier case is esm

1db6060

fixup: doc lint issues

1cb5cea

MylesBorins force-pushed the resolution-flag branch from a7b4b27 to 1cb5cea Compare March 9, 2019 01:41

MylesBorins closed this Mar 9, 2019

ljharb deleted the resolution-flag branch March 9, 2019 06:41

GeoffreyBooth changed the title ~~[WIP] --es-module-specifier-resolution flag~~ --es-module-specifier-resolution flag Mar 13, 2019

GeoffreyBooth mentioned this pull request Mar 13, 2019

File extension/directory index resolution in ESM nodejs/modules#268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--es-module-specifier-resolution flag #48

--es-module-specifier-resolution flag #48

MylesBorins commented Mar 5, 2019

thysultan commented Mar 5, 2019

guybedford commented Mar 5, 2019

devsnek commented Mar 5, 2019 •

edited

Loading

ljharb commented Mar 5, 2019

This comment has been minimized.

MylesBorins commented Mar 6, 2019

MylesBorins Mar 6, 2019

MylesBorins commented Mar 8, 2019 •

edited

Loading

MylesBorins commented Mar 9, 2019

--es-module-specifier-resolution flag #48

--es-module-specifier-resolution flag #48

Conversation

MylesBorins commented Mar 5, 2019

thysultan commented Mar 5, 2019

guybedford commented Mar 5, 2019

devsnek commented Mar 5, 2019 • edited Loading

ljharb commented Mar 5, 2019

This comment has been minimized.

MylesBorins commented Mar 6, 2019

MylesBorins Mar 6, 2019

Choose a reason for hiding this comment

MylesBorins commented Mar 8, 2019 • edited Loading

MylesBorins commented Mar 9, 2019

devsnek commented Mar 5, 2019 •

edited

Loading

MylesBorins commented Mar 8, 2019 •

edited

Loading