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.

Sign up for GitHub

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

Jump to bottom

esm: improve error message of ERR_UNSUPPORTED_ESM_URL_SCHEME #34795

Closed
wants to merge 1 commit into from
Closed

esm: improve error message of ERR_UNSUPPORTED_ESM_URL_SCHEME #34795

wants to merge 1 commit into from

Conversation

lundibundi
Copy link
Member

Refs: #34765

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

/cc @nodejs/modules-active-members

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@nodejs-github-bot 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 Aug 16, 2020
@nodejs-github-bot
Copy link
Collaborator

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

mcollina
mcollina approved these changes Aug 16, 2020
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

DerekNonGeneric
DerekNonGeneric reviewed Aug 18, 2020
View reviewed changes
lib/internal/errors.js
'by the default ESM loader', Error);
E('ERR_UNSUPPORTED_ESM_URL_SCHEME', (url) => {
let msg = 'Only file and data URLs are supported by the default ESM loader';
if (isWindows && url.protocol.length === 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would the protocol length of two be what triggers this? Wouldn't it at the very least be > 1?

Copy link
Member

Choose a reason for hiding this comment

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

.protocol includes the :

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, so URL('C:\\example\\foo.mjs') would have .protocol === 'c:'. This covers only such use-case for now.

@lundibundi lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 18, 2020
@lundibundi
Copy link
Member Author

For some reason, bot didn't post the CI

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

@Trott
Copy link
Member

Trott commented Aug 19, 2020

Landed in a084632

@Trott Trott closed this Aug 19, 2020
Trott pushed a commit that referenced this pull request Aug 19, 2020
@lundibundi @Trott
esm: improve error message of ERR_UNSUPPORTED_ESM_URL_SCHEME
a084632 
Refs: #34765

PR-URL: #34795
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@lundibundi lundibundi deleted the improve-esm-error-msg branch August 19, 2020 10:03
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
@lundibundi @BethGriggs
esm: improve error message of ERR_UNSUPPORTED_ESM_URL_SCHEME
f4675ea 
Refs: #34765

PR-URL: #34795
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@danielleadams danielleadams mentioned this pull request Aug 20, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
@lundibundi @BethGriggs
esm: improve error message of ERR_UNSUPPORTED_ESM_URL_SCHEME
8f38c19 
Refs: #34765

PR-URL: #34795
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
guybedford pushed a commit to guybedford/node that referenced this pull request Sep 28, 2020
@lundibundi @guybedford
esm: improve error message of ERR_UNSUPPORTED_ESM_URL_SCHEME
8030eb5 
Refs: nodejs#34765

PR-URL: nodejs#34795
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@lundibundi @codebytere
esm: improve error message of ERR_UNSUPPORTED_ESM_URL_SCHEME
551be2a 
Refs: #34765

PR-URL: #34795
Backport-PR-URL: #35385
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@codebytere codebytere mentioned this pull request Oct 4, 2020
Merged
@jasnell jasnell mentioned this pull request Jan 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DerekNonGeneric DerekNonGeneric DerekNonGeneric left review comments

@bmeck bmeck bmeck approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@guybedford guybedford guybedford approved these changes

@jkrems jkrems jkrems approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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.
10 participants
@lundibundi @nodejs-github-bot @Trott @mcollina @bmeck @jkrems @guybedford @addaleax @BridgeAR @DerekNonGeneric