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: improve error message of ERR_UNSUPPORTED_ESM_URL_SCHEME #34795

Closed
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

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

'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
Refs: #34765

PR-URL: #34795
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@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
Refs: #34765

PR-URL: #34795
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request Aug 20, 2020
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
Refs: #34765

PR-URL: #34795
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
guybedford pushed a commit to guybedford/node that referenced this pull request Sep 28, 2020
Refs: nodejs#34765

PR-URL: nodejs#34795
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
Refs: #34765

PR-URL: #34795
Backport-PR-URL: #35385
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Oct 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

10 participants