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

esm: modify resolution order for specifier flag #29974

Closed
wants to merge 2 commits into from

Conversation

@MylesBorins
Copy link
Member

MylesBorins commented Oct 14, 2019

Currently --es-module-specifier-resolution=node has an alternative
resolution order than the default in common.js, this causes inconsistencies.
As discussed in @nodejs/modules we want to preserve resolution order between
implementations.

@MylesBorins MylesBorins requested review from guybedford, devsnek and jkrems Oct 14, 2019
@MylesBorins MylesBorins referenced this pull request Oct 14, 2019
14 of 14 tasks complete
@nodejs-github-bot

This comment was marked as outdated.

@jkrems
jkrems approved these changes Oct 14, 2019
".js",
".json",
".node"
".node",
".mjs",

This comment has been minimized.

Copy link
@jkrems

jkrems Oct 14, 2019

Contributor

I don't think we check these in CJS. There is some risk that this leads to two valid resolutions, especially when combined with require hooks:

  • foo.jsx or foo.ts: Found by CJS.
  • foo.mjs or foo.cjs: Found by ESM.

Should we remove them? I think I'm fine keeping them since require hooks aren't "native".

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Oct 14, 2019

Author Member

Don't think we should remove them personally, but maybe I'm not entirely understanding the case here

This comment has been minimized.

Copy link
@DerekNonGeneric

DerekNonGeneric Oct 14, 2019

Should we remove them?

This brings up a good point about whether we should expect the resolution outcomes for --es-module-specifier-resolution=node and require.resolve() to be identical.

The rollup-plugin-node-resolve package refers to the current order of extension resolution as node (it's in the name).

Maybe a new mode of legacy (--es-module-specifier-resolution=legacy) should be introduced to function exactly like require.resolve(). The ESM package refers to it as legacy internally.

This comment has been minimized.

Copy link
@weswigham

weswigham Oct 14, 2019

Erm, couldn't we choose to pull the extension list and order from require.extensions directly, for compat? Even if it's the pre-user-code-modified version (therefore didn't support custom extensions), it'd at least guarantee they're in sync.

This comment has been minimized.

Copy link
@mcollina

mcollina Oct 15, 2019

Member

What will be the consequences of removing?

This comment has been minimized.

Copy link
@MylesBorins

MylesBorins Oct 15, 2019

Author Member

So i've gone ahead and left .mjs extension and removed the .cjs extension. Thought process here is that the cjs loader doesn't recognize that extension, so lets keep things consistent. They might be a bit weird... but at least they are primarily the same.

The CJS loader will currently fail on requiring a .mjs extension, not pass it through... so I think the behavior is equivalent.

This will pass through anything that doesn't match to the CJS loader, which in turn will get anything from require.extensions.

I believe this covers all the concerns raised in this thread

This comment has been minimized.

Copy link
@DerekNonGeneric

DerekNonGeneric Oct 16, 2019

I think removing the .cjs extension is a good start as I personally believe that the node resolution mode should function exactly as described in the All Together... algorithm.

In defense of modifying the behavior of this flag, the blog post that announced this flag made clear that its behavior would change once the ES module context went live unflagged, so removing or changing extension precedence hopefully won't come as too much of a shock to most.

As long as the --loader flag will continue to be available, I don't think anyone can justify being too upset about changes like this since the option of supplying one's own resolution algorithm is still on the table.

Copy link
Contributor

guybedford left a comment

Makes sense.

@MylesBorins

This comment has been minimized.

Copy link
Member Author

MylesBorins commented Oct 15, 2019

I just pushed a super naive solution to require.extensions

TL;DR since we currently error if file extension don't match anything in the map I'm alternatively forwarding any unmatched extensions to the .cjs loader to be resolved. This will mean that any ambiguous (to esm loader) file extension will be forwarded and eventually default to CJS.

We could potentially find a more elegant solution that doesn't involve a CJS default, but it is going to require FAR more juggling since the extensions all exist in JS land and the extension resolution for the ESM loader is in C++.

If folks are open to this solution I'll write some tests and docs.

I've found a potential regression in the loader related to requiring a directory ... gonna dig into that rn

edit: sounds like guy's refactoring of the bootstrap actually fixes this regression... I've written a test and pushed to his branch

edit 2: guy's branch still has the bug... will work on fixing it there

Copy link
Contributor

guybedford left a comment

Re-approved with latest changes FWIW.

@nodejs-github-bot

This comment was marked as outdated.

MylesBorins added 2 commits Oct 14, 2019
Currently `--es-module-specifier-resolution=node` has an alternative
resolution order than the default in common.js, this causes
inconsistencies. As discussed in @nodejs/modules we want to preserve
resolution order between implementations.
@MylesBorins MylesBorins force-pushed the MylesBorins:es-resolution-order branch from 9467d7f to 935e46c Oct 15, 2019
@nodejs-github-bot

This comment has been minimized.

guybedford added a commit that referenced this pull request Oct 17, 2019
Currently `--es-module-specifier-resolution=node` has an alternative
resolution order than the default in common.js, this causes
inconsistencies. As discussed in @nodejs/modules we want to preserve
resolution order between implementations.

PR-URL: #29974
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@guybedford

This comment has been minimized.

Copy link
Contributor

guybedford commented Oct 17, 2019

Landed in 1e5ed9a.

@guybedford guybedford closed this Oct 17, 2019
@richardlau richardlau referenced this pull request Oct 17, 2019
3 of 3 tasks complete
MylesBorins added a commit that referenced this pull request Oct 23, 2019
Currently `--es-module-specifier-resolution=node` has an alternative
resolution order than the default in common.js, this causes
inconsistencies. As discussed in @nodejs/modules we want to preserve
resolution order between implementations.

PR-URL: #29974
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Oct 23, 2019
Currently `--es-module-specifier-resolution=node` has an alternative
resolution order than the default in common.js, this causes
inconsistencies. As discussed in @nodejs/modules we want to preserve
resolution order between implementations.

PR-URL: #29974
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins referenced this pull request Oct 23, 2019
targos added a commit that referenced this pull request Oct 23, 2019
Notable changes:

* deps:
  * Fixed a bug in npm 6.12.0 where warnings are emitted on Node.js
    13.x. #30079
* esm:
  * Changed file extension resolution order of
    `--es-module-specifier-resolution=node`to match that of the CommonJS
    loader. #29974

PR-URL: #30081
targos added a commit that referenced this pull request Oct 23, 2019
Notable changes:

* deps:
  * Fixed a bug in npm 6.12.0 where warnings are emitted on Node.js
    13.x. #30079
* esm:
  * Changed file extension resolution order of
    `--es-module-specifier-resolution=node`to match that of the CommonJS
    loader. #29974

PR-URL: #30081
targos added a commit that referenced this pull request Nov 8, 2019
Currently `--es-module-specifier-resolution=node` has an alternative
resolution order than the default in common.js, this causes
inconsistencies. As discussed in @nodejs/modules we want to preserve
resolution order between implementations.

PR-URL: #29974
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request Nov 10, 2019
Currently `--es-module-specifier-resolution=node` has an alternative
resolution order than the default in common.js, this causes
inconsistencies. As discussed in @nodejs/modules we want to preserve
resolution order between implementations.

PR-URL: #29974
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.