Skip to content

esm: improve error message of ERR_UNSUPPORTED_ESM_URL_SCHEME#34795

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
Closed
lundibundi wants to merge 1 commit into from
Closed

esm: improve error message of ERR_UNSUPPORTED_ESM_URL_SCHEME #34795

lundibundi wants to merge 1 commit into from

Conversation

@lundibundi
Copy link
Member

@lundibundilundibundi commented Aug 16, 2020

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

nodejs-github-bot commented Aug 16, 2020

Review requested:

  • @nodejs/modules

@nodejs-github-botnodejs-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

nodejs-github-bot commented Aug 16, 2020

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

@mcollinamcollina 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)=>{
letmsg='Only file and data URLs are supported by the default ESM loader';
if(isWindows&&url.protocol.length===2){
Copy link
Contributor

@DerekNonGenericDerekNonGenericAug 18, 2020

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

@bmeckbmeckAug 18, 2020

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
MemberAuthor

@lundibundilundibundiAug 18, 2020

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.

@lundibundilundibundi 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
MemberAuthor

lundibundi commented Aug 18, 2020

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

@TrottTrott 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]>
@lundibundilundibundi 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]>
@danielleadamsdanielleadams 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]>
@codebyterecodebytere mentioned this pull request Oct 4, 2020
Merged
@jasnelljasnell mentioned this pull request Jan 5, 2021
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes

@hybristhybristhybrist approved these changes

@guybedfordguybedfordguybedford approved these changes

@TrottTrottTrott approved these changes

@addaleaxaddaleaxaddaleax approved these changes

@BridgeARBridgeARBridgeAR approved these changes

+2 more reviewers

@bmeckbmeckbmeck approved these changes

@DerekNonGenericDerekNonGenericDerekNonGeneric left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.errorsIssues and PRs related to JavaScript errors originated in Node.js core.esmIssues 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@hybrist@guybedford@addaleax@BridgeAR@DerekNonGeneric