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: shorten ERR_UNSUPPORTED_ESM_URL_SCHEME message #34836

Merged
merged 1 commit into from
Aug 30, 2020
Merged

esm: shorten ERR_UNSUPPORTED_ESM_URL_SCHEME message #34836

merged 1 commit into from
Aug 30, 2020

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 19, 2020

I know it just got modified to include new information, but this
shortens the message a bit without (I hope) losing clarity or meaning.

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

@nodejs-github-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Aug 19, 2020
@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 19, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 19, 2020
DerekNonGeneric
DerekNonGeneric previously requested changes Aug 19, 2020
View reviewed changes
Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I believe these are technically called DOS paths, but having this say Windows paths is probably ((maybe?)) fine since I don't think anyone is running Node under DOS.

Refs: https://en.wikipedia.org/wiki/DOS

test/es-module/test-esm-dynamic-import.js Outdated Show resolved Hide resolved
@Trott

This comment has been minimized.

Sign in to view
lundibundi
lundibundi reviewed Aug 19, 2020
View reviewed changes
lib/internal/errors.js Outdated Show resolved Hide resolved
richardlau
richardlau reviewed Aug 19, 2020
View reviewed changes
lib/internal/errors.js Outdated Show resolved Hide resolved
@Trott
Copy link
Member Author

Trott commented Aug 21, 2020

I think all comments have been addressed. Assuming CI passes on Windows, PTAL.

@Trott
Copy link
Member Author

Trott commented Aug 21, 2020

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

(Not sure if the bot is working again.)

@github-actions github-actions bot mentioned this pull request Aug 21, 2020
Open
9 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@mmarchini mmarchini mentioned this pull request Aug 21, 2020
Closed
@Trott
Copy link
Member Author

Trott commented Aug 21, 2020

@DerekNonGeneric Can you please either clear your change request or explain it a bit more? Are you asking that we change the text? Or was the change request about the missing single-quotation mark that has been added?

@DerekNonGeneric
Copy link
Contributor

I think it's technically wrong, but do as you please.

@DerekNonGeneric
Copy link
Contributor

@Trott, can you dismiss my stale review?

@Trott
Copy link
Member Author

Trott commented Aug 21, 2020

I think it's technically wrong, but do as you please.

Well, let's see if we can come up with something that satisfies you and @lundibundi and anyone else invested.

Maybe we don't need to mention Windows specifically at all?

Absolute paths need to be prefixed with file://.

It will only display for Windows users, and that's fine (because that's who needs to see it in this situation).

Does that work for everyone?

@github-actions github-actions bot mentioned this pull request Aug 22, 2020
Open
9 tasks
@Trott Trott dismissed DerekNonGeneric’s stale review August 22, 2020 13:30

cleared per request

@DerekNonGeneric
Copy link
Contributor

Thank you, you are the best.

This was referenced Aug 23, 2020
Open
Open
@nodejs-github-bot

This comment has been minimized.

Sign in to view
This was referenced Aug 25, 2020
Open
Open
Open
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

@Trott
esm: shorten ERR_UNSUPPORTED_ESM_URL_SCHEME message
e1edd6b 
I know it just got modified to include new information, but this
shortens the message a bit without (I hope) losing clarity or meaning.

PR-URL: nodejs#34836
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@Trott
Copy link
Member Author

Trott commented Aug 30, 2020

Landed in e1edd6b

@Trott Trott merged commit e1edd6b into nodejs:master Aug 30, 2020
@Trott Trott deleted the err-esm branch August 30, 2020 16:19
@GulajavaMinistudio GulajavaMinistudio mentioned this pull request Aug 31, 2020
Merged
4 tasks
richardlau pushed a commit that referenced this pull request Sep 1, 2020
@Trott @richardlau
esm: shorten ERR_UNSUPPORTED_ESM_URL_SCHEME message
db8c66b 
I know it just got modified to include new information, but this
shortens the message a bit without (I hope) losing clarity or meaning.

PR-URL: #34836
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@richardlau richardlau mentioned this pull request Sep 2, 2020
Merged
4 tasks
guybedford pushed a commit to guybedford/node that referenced this pull request Sep 28, 2020
@Trott @guybedford
esm: shorten ERR_UNSUPPORTED_ESM_URL_SCHEME message
96325b1 
I know it just got modified to include new information, but this
shortens the message a bit without (I hope) losing clarity or meaning.

PR-URL: nodejs#34836
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
codebytere pushed a commit that referenced this pull request Oct 1, 2020
@Trott @codebytere
esm: shorten ERR_UNSUPPORTED_ESM_URL_SCHEME message
9159649 
I know it just got modified to include new information, but this
shortens the message a bit without (I hope) losing clarity or meaning.

PR-URL: #34836
Backport-PR-URL: #35385
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@codebytere codebytere mentioned this pull request Oct 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi approved these changes

@richardlau richardlau richardlau left review comments

@jasnell jasnell jasnell left review comments

@guybedford guybedford guybedford approved these changes

@DerekNonGeneric DerekNonGeneric Awaiting requested review from DerekNonGeneric

Assignees
No one assigned
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Trott @nodejs-github-bot @DerekNonGeneric @jasnell @guybedford @richardlau @lundibundi @addaleax