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: data URLs should ignore unknown parameters #30593

Closed
wants to merge 1 commit into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Nov 22, 2019

The data: URL implementation was overly strict about what MIMEs it accepted. This caused problems for people using parameters. This relaxes the restrictions and follows the MIME expectation of ignoring unknown parameters. Per RFC2045:

MIME implementations must ignore any parameters whose names they do not recognize.

This also matches the WHATWG handling of data: URLs by using any , as the splitting point.

CC: @nodejs/modules

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

@bmeck bmeck added experimental Issues and PRs related to experimental features. esm Issues and PRs related to the ECMAScript Modules implementation. labels Nov 22, 2019
@bmeck bmeck requested a review from guybedford November 22, 2019 16:41
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Great, thank you.

@SMotaal
Copy link

SMotaal commented Nov 22, 2019

For reference (from the original PR) — https://gist.github.com/SMotaal/2e0ede8c6115b828c111fa84a1e952b4

@bmeck bmeck force-pushed the esm-data-url-params branch from 415695b to 2495ac6 Compare November 26, 2019 14:58
@bmeck bmeck force-pushed the esm-data-url-params branch from 2495ac6 to 7a6a6db Compare November 26, 2019 14:58
@nodejs-github-bot
Copy link
Collaborator

@bmeck
Copy link
Member Author

bmeck commented Nov 26, 2019

CI error is on waiting for AIX executor timeout, looks good to merge in a few

bmeck pushed a commit that referenced this pull request Nov 26, 2019
PR-URL: #30593
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@bmeck
Copy link
Member Author

bmeck commented Nov 26, 2019

Landed in 568968e

@bmeck bmeck closed this Nov 26, 2019
addaleax pushed a commit that referenced this pull request Nov 30, 2019
PR-URL: #30593
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
MylesBorins pushed a commit that referenced this pull request Jan 12, 2020
PR-URL: #30593
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
PR-URL: #30593
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants