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

module: improve error message for invalid data URL by aduh95 · Pull Request #37701 · nodejs/node · GitHub

/ node Public

module: improve error message for invalid data URL - #37701

Merged
aduh95 merged 1 commit into
nodejs:masterfrom
aduh95:invalid-mime-data-url
Apr 3, 2021
ConversationCommits (1)ChecksFiles changed
Merged

module: improve error message for invalid data URL#37701
aduh95 merged 1 commit into
nodejs:masterfrom
aduh95:invalid-mime-data-url

Conversation

aduh95 commented Mar 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Fixes: #37647

Changes the error message when importing module with an unknown/unsupported MIME type from:

TypeError [ERR_INVALID_RETURN_PROPERTY_VALUE]: Expected string to be returned for the "format" from the "loader getFormat" function but got type object.

to

TypeError [ERR_INVALID_MODULE_SPECIFIER]: Invalid module "data:application/json,[]" has an unsupported MIME type "application/json"

aduh95 force-pushed the invalid-mime-data-url branch from 114e4c8 to 68fcf83 Compare March 10, 2021 22:24
nodejs-github-bot added the needs-ci PRs that need a full CI run. label Mar 10, 2021

targos commented Mar 12, 2021

Copy link
Copy Markdown
Member

@nodejs/modules

targos added the esm Issues and PRs related to the ECMAScript Modules implementation. label Mar 12, 2021
DerekNonGeneric reviewed Mar 12, 2021
View reviewed changes
Comment thread lib/internal/modules/esm/loader.js Outdated
DerekNonGeneric reviewed Mar 12, 2021
View reviewed changes

DerekNonGeneric 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

Just my 2¢. Not sure that error code has been used for this purpose historically.

Comment thread lib/internal/modules/esm/loader.js Outdated

bmeck commented Mar 12, 2021

Copy link
Copy Markdown
Member

CC: @ljharb since this mentions MIME to user

bmeck commented Mar 12, 2021

Copy link
Copy Markdown
Member

to clarify a bit, these should have been picked up and could have been generated in the past

ljharb commented Mar 12, 2021

Copy link
Copy Markdown
Member

Seems fine to me, since anyone using a data URL is already familiar with MIMEs (this is ofc not true for almost every other specifier category).

aduh95 added the review wanted PRs that need reviews. label Mar 21, 2021

aduh95 commented Apr 2, 2021

Copy link
Copy Markdown
Contributor Author

Ping @nodejs/modules for reviews.

DerekNonGeneric approved these changes Apr 2, 2021
View reviewed changes

DerekNonGeneric 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

Aside from my outstanding comments (non-blocking), everything else looks pretty good to me.

I will be digging around this part of codebase in a few days, so I will report back if I find issues.

Comment thread lib/internal/modules/esm/loader.js Outdated
Comment thread test/es-module/test-esm-invalid-data-urls.js Outdated
guybedford approved these changes Apr 2, 2021
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

Perhaps Unsupported MIME "text/plain" loading "data:text/plain,export default 0".

Also it could be nice if we could support the parent context for tracing where it comes from eg if it were import('data:...') in a module, it helps to know where the import expression is.

aduh95 commented Apr 2, 2021

Copy link
Copy Markdown
Contributor Author

@guybedford Agreed, although I think this would deserve its own PR to align with other ERR_INVALID_MODULE_SPECIFIER uses in core.

aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 2, 2021

nodejs-github-bot commented Apr 2, 2021

Copy link
Copy Markdown
Collaborator

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

guybedford commented Apr 2, 2021

Copy link
Copy Markdown
Contributor

@aduh95 I do think it's important to move the "unsupported MIME" part to the beginning of the message though. This is because the data URL could be of any length, so that when scanning the error the user should be able to easily see the "unsupported MIME" part first. It also fixes that the current error isn't quite gramatically complete without a comma or colon or something type of break otherwise.

Alternatively to make it gramatically correct:

  • Use a full stop and a new sentence - The MIME type "..." is incorrect.
  • Use a comma / colon / dash.
  • Use "has an unsupported MIME type "...""

nodejs-github-bot commented Apr 2, 2021

Copy link
Copy Markdown
Collaborator

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

aduh95 commented Apr 2, 2021

Copy link
Copy Markdown
Contributor Author

I went to the last suggestion as it was the easiest to implement. I think you have a point regarding very long specifiers occulting the reason, maybe we can fix that in a similar way as proposed in #37852 (comment)?

DerekNonGeneric commented Apr 2, 2021

Copy link
Copy Markdown
Contributor

[…] maybe we can fix that in a similar way as proposed in #37852 (comment)?

I believe the feature request outlined in #37581 is precisely the error property we need.

nodejs-github-bot commented Apr 2, 2021

Copy link
Copy Markdown
Collaborator

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

module: improve error message for invalid data URL
46062a0 
Fixes: nodejs#37647

PR-URL: nodejs#37701
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
aduh95 force-pushed the invalid-mime-data-url branch from f5ed39f to 46062a0 Compare April 3, 2021 18:06

aduh95 commented Apr 3, 2021

Copy link
Copy Markdown
Contributor Author

Landed in 46062a0

aduh95 merged commit 46062a0 into nodejs:master Apr 3, 2021
aduh95 deleted the invalid-mime-data-url branch April 3, 2021 18:06
MylesBorins pushed a commit that referenced this pull request Apr 4, 2021
module: improve error message for invalid data URL
618da81 
Fixes: #37647

PR-URL: #37701
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
MylesBorins mentioned this pull request Apr 4, 2021
Merged
MylesBorins pushed a commit that referenced this pull request Apr 5, 2021
module: improve error message for invalid data URL
3ab9619 
Fixes: #37647

PR-URL: #37701
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
targos pushed a commit that referenced this pull request May 1, 2021
module: improve error message for invalid data URL
079671d 
Fixes: #37647

PR-URL: #37701
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
danielleadams mentioned this pull request May 3, 2021
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

guybedford guybedford approved these changes
+1 more reviewer
DerekNonGeneric DerekNonGeneric 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. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. question Issues that look for answers. review wanted PRs that need reviews.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Import from JSON data URL doesn't work

7 participants

Back | FazBrowse Home | New Git URL