FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

esm: modify resolution order for specifier flag by MylesBorins · Pull Request #29974 · nodejs/node · GitHub

/ node Public

esm: modify resolution order for specifier flag - #29974

Closed
MylesBorins wants to merge 2 commits into
nodejs:masterfrom
MylesBorins:es-resolution-order
ConversationCommits (2)ChecksFiles changed
Closed

esm: modify resolution order for specifier flag#29974
MylesBorins wants to merge 2 commits into
nodejs:masterfrom
MylesBorins:es-resolution-order

Conversation

MylesBorins commented Oct 14, 2019

Copy link
Copy Markdown
Contributor

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.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 14, 2019
MylesBorins mentioned this pull request Oct 14, 2019
Closed
14 tasks

This comment has been minimized.

Sign in to view
hybrist approved these changes Oct 14, 2019
View reviewed changes
Comment thread src/module_wrap.cc Outdated

hybrist Oct 14, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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".

MylesBorins Oct 14, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

DerekNonGeneric Oct 14, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

weswigham Oct 14, 2019

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

mcollina Oct 15, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What will be the consequences of removing?

MylesBorins Oct 15, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

DerekNonGeneric Oct 16, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

guybedford approved these changes Oct 14, 2019
View reviewed changes

guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Makes sense.

MylesBorins added the esm Issues and PRs related to the ECMAScript Modules implementation. label Oct 15, 2019

MylesBorins commented Oct 15, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

guybedford approved these changes Oct 15, 2019
View reviewed changes

guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Re-approved with latest changes FWIW.

This comment has been minimized.

Sign in to view
esm: modify resolution order for specifier flag
8762750 
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.
fixup: naive solution to require.extensions
935e46c

nodejs-github-bot commented Oct 15, 2019

Copy link
Copy Markdown
Collaborator

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

guybedford pushed a commit that referenced this pull request Oct 17, 2019
esm: modify resolution order for specifier flag
1e5ed9a 
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 commented Oct 17, 2019

Copy link
Copy Markdown
Contributor

Landed in 1e5ed9a.

guybedford closed this Oct 17, 2019
richardlau mentioned this pull request Oct 17, 2019
Closed
3 tasks
MylesBorins added a commit that referenced this pull request Oct 23, 2019
esm: modify resolution order for specifier flag
9729c5d 
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 mentioned this pull request Oct 23, 2019
Merged
targos pushed a commit that referenced this pull request Oct 23, 2019
2019-10-23, Version 13.0.1 (Current)
bdc09c6 
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 pushed a commit that referenced this pull request Oct 23, 2019
2019-10-23, Version 13.0.1 (Current)
5a042a6 
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 pushed a commit that referenced this pull request Nov 8, 2019
esm: modify resolution order for specifier flag
434e5ce 
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 pushed a commit that referenced this pull request Nov 10, 2019
esm: modify resolution order for specifier flag
94ac44f 
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 mentioned this pull request Nov 10, 2019
Merged

jaydenseric commented Nov 18, 2019

Copy link
Copy Markdown
Contributor

I think this change might be causing this issue: #30520

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina left review comments
jasnell jasnell approved these changes
hybrist hybrist approved these changes
guybedford guybedford approved these changes
devsnek Awaiting requested review from devsnek
+2 more reviewers
weswigham weswigham left review comments
DerekNonGeneric DerekNonGeneric left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL