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

esm: better error message for unsupported URL by Hakerh400 · Pull Request #31129 · nodejs/node · GitHub

/ node Public

esm: better error message for unsupported URL - #31129

Closed
Hakerh400 wants to merge 1 commit into
nodejs:masterfrom
Hakerh400:esm-url
ConversationCommits (1)ChecksFiles changed
Closed

esm: better error message for unsupported URL#31129
Hakerh400 wants to merge 1 commit into
nodejs:masterfrom
Hakerh400:esm-url

Conversation

Hakerh400 commented Dec 29, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

The default ESM loader supports only file and data URLs.
This PR adds better error message for it.

Fixes #31120

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. esm Issues and PRs related to the ECMAScript Modules implementation. labels Dec 29, 2019

Hakerh400 commented Dec 29, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

test/es-module/test-esm-dynamic-import.js

// TODO(jkrems): Right now this doesn't hit a protocol error because the
// module resolution step already rejects it. These arguably should be
// protocol errors.
expectMissingModuleError(import('node:fs'));
expectMissingModuleError(import('http://example.com/foo.js'));

This is the only test failing right now. The problem is that the loader converts the url to a file url and ignores the url scheme, producing a misleading error message (and it may probably load a wrong module). Should we update this test to check for ERR_UNSUPPORTED_ESM_URL_SCHEME, or should we delete this new error type and stick with the generic ERR_MODULE_NOT_FOUND?

devsnek suggested changes Dec 29, 2019
View reviewed changes

devsnek left a comment

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

you can use parsed.scheme a few lines down.

devsnek reviewed Dec 29, 2019
View reviewed changes
Comment thread lib/internal/modules/esm/default_resolve.js Outdated
Trott reviewed Dec 29, 2019
View reviewed changes
Comment thread doc/api/errors.md Outdated
guybedford approved these changes Dec 29, 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

Thanks for the PR, the approach looks great.

The test failures can be fixed by just changing the test expectations in https://github.com/nodejs/node/blob/master/test/es-module/test-esm-dynamic-import.js#L60. Note that the errors exactly align with a TODO comment @jkrems included previously.

esm: better error message for unsupported URL
b03912b 
The default ESM loader supports only file and data URLs.
This PR adds better error message for it.

nodejs-github-bot commented Dec 30, 2019

Copy link
Copy Markdown
Collaborator

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

Trott commented Dec 31, 2019

Copy link
Copy Markdown
Member

This is stalled on two issues in CI (nodejs/build#2116 and nodejs/build#2115) that will hopefully be resolved soon.

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 2, 2020

nodejs-github-bot commented Jan 2, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jan 2, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Jan 3, 2020

Copy link
Copy Markdown
Collaborator

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

BridgeAR commented Jan 3, 2020

Copy link
Copy Markdown
Member

Landed in a25a628 🎉

BridgeAR closed this Jan 3, 2020
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 3, 2020
esm: better error message for unsupported URL
a25a628 
The default ESM loader supports only file and data URLs.
This PR adds better error message for it.

PR-URL: nodejs#31129
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Hakerh400 deleted the esm-url branch January 3, 2020 14:50
BridgeAR pushed a commit that referenced this pull request Jan 3, 2020
esm: better error message for unsupported URL
a6c2502 
The default ESM loader supports only file and data URLs.
This PR adds better error message for it.

PR-URL: #31129
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR mentioned this pull request Jan 7, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
esm: better error message for unsupported URL
c10ee5d 
The default ESM loader supports only file and data URLs.
This PR adds better error message for it.

PR-URL: #31129
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
esm: better error message for unsupported URL
a2ac9d3 
The default ESM loader supports only file and data URLs.
This PR adds better error message for it.

PR-URL: #31129
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins mentioned this pull request Feb 8, 2020
Merged
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

jasnell jasnell approved these changes
guybedford guybedford approved these changes
Trott Trott approved these changes
lpinca lpinca approved these changes
BridgeAR BridgeAR approved these changes
+1 more reviewer
devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core. 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.

import does not work correctly

8 participants

Back | FazBrowse Home | New Git URL