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

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

Closed
wants to merge 6 commits into from
Closed

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

wants to merge 6 commits into from

Conversation

@MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Mar 5, 2019

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

@MylesBorins MylesBorins changed the title [WIP] Legacy Resolution Falg [WIP] Legacy Resolution Flag Mar 5, 2019
@MylesBorins MylesBorins mentioned this pull request Mar 5, 2019
Closed
12 tasks
@thysultan
Copy link

thysultan commented Mar 5, 2019

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
Copy link
Contributor

guybedford commented Mar 5, 2019

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
Copy link
Member

devsnek commented Mar 5, 2019 •
edited
Loading

@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
Copy link
Member

ljharb commented Mar 5, 2019

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.

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

This comment has been minimized.

Sign in to view
@MylesBorins MylesBorins changed the title [WIP] --esm-extension-resolution flag [WIP] --esm-filename-resolution flag Mar 5, 2019
GeoffreyBooth
GeoffreyBooth suggested changes Mar 6, 2019
View reviewed changes
@MylesBorins MylesBorins added the enhancement New feature or request label Mar 6, 2019
GeoffreyBooth
GeoffreyBooth reviewed Mar 6, 2019
View reviewed changes
@MylesBorins MylesBorins changed the title [WIP] --esm-filename-resolution flag [WIP] --es-module-specifier-resolution flag Mar 6, 2019
@MylesBorins MylesBorins force-pushed the resolution-flag branch 4 times, most recently from a17b130 to 3e3bb27 Compare March 6, 2019 07:46
@MylesBorins
Copy link
Contributor Author

MylesBorins commented Mar 6, 2019

Updated based on all notes and added extensive tests PTAL

GeoffreyBooth
GeoffreyBooth previously requested changes Mar 6, 2019
View reviewed changes
MylesBorins
MylesBorins commented Mar 6, 2019
View reviewed changes
test/es-module/test-esm-specifiers.mjs
assert.strictEqual(success, 'success');
assert.strictEqual(explicit, 'esm');
assert.strictEqual(implicit, 'cjs');
assert.strictEqual(implicit, 'esm');
Copy link
Contributor Author

@MylesBorins MylesBorins Mar 6, 2019

Choose a reason for hiding this comment

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

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

@GeoffreyBooth GeoffreyBooth dismissed their stale review March 8, 2019 22:57

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

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Mar 8, 2019 •
edited
Loading

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/

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Mar 9, 2019

landed in bec588f

@MylesBorins MylesBorins closed this Mar 9, 2019
@ljharb ljharb deleted the resolution-flag branch March 9, 2019 06:41
@GeoffreyBooth GeoffreyBooth changed the title [WIP] --es-module-specifier-resolution flag --es-module-specifier-resolution flag Mar 13, 2019
@GeoffreyBooth GeoffreyBooth mentioned this pull request Mar 13, 2019
Closed
nodejs-ci pushed a commit that referenced this pull request Aug 21, 2019
@isaacs @Trott
deps: update npm to 6.10.3
f769034 
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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@ljharb ljharb ljharb left review comments

@GeoffreyBooth GeoffreyBooth GeoffreyBooth left review comments

@devsnek devsnek devsnek left review comments

@guybedford guybedford guybedford approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@MylesBorins @thysultan @guybedford @devsnek @ljharb @GeoffreyBooth