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 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
@jkrems
jkrems approved these changes Nov 22, 2019
Copy link
Contributor

guybedford left a comment

Great, thank you.

@SMotaal

This comment has been minimized.

Copy link
Member

SMotaal commented Nov 22, 2019

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

@bmeck bmeck force-pushed the bmeck:esm-data-url-params branch Nov 26, 2019
@bmeck bmeck force-pushed the bmeck:esm-data-url-params branch to 7a6a6db Nov 26, 2019
@nodejs-github-bot

This comment has been minimized.

@bmeck

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

bmeck commented Nov 26, 2019

Landed in 568968e

@bmeck bmeck closed this Nov 26, 2019
addaleax added 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 added 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 added 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
saitonakamura pushed a commit to saitonakamura/node that referenced this pull request Apr 7, 2020
PR-URL: nodejs#30593
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.