-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: support loading data: URLs #28614
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
// Flags: --experimental-modules | ||
'use strict'; | ||
const common = require('../common'); | ||
const assert = require('assert'); | ||
function createURL(mime, body) { | ||
return `data:${mime},${body}`; | ||
} | ||
function createBase64URL(mime, body) { | ||
return `data:${mime};base64,${Buffer.from(body).toString('base64')}`; | ||
} | ||
(async () => { | ||
{ | ||
const body = 'export default {a:"aaa"};'; | ||
const plainESMURL = createURL('text/javascript', body); | ||
const ns = await import(plainESMURL); | ||
assert.deepStrictEqual(Object.keys(ns), ['default']); | ||
assert.deepStrictEqual(ns.default.a, 'aaa'); | ||
const importerOfURL = createURL( | ||
'text/javascript', | ||
`export {default as default} from ${JSON.stringify(plainESMURL)}` | ||
); | ||
assert.strictEqual( | ||
(await import(importerOfURL)).default, | ||
ns.default | ||
); | ||
const base64ESMURL = createBase64URL('text/javascript', body); | ||
assert.notStrictEqual( | ||
await import(base64ESMURL), | ||
ns | ||
); | ||
} | ||
{ | ||
const body = 'export default import.meta.url;'; | ||
const plainESMURL = createURL('text/javascript', body); | ||
const ns = await import(plainESMURL); | ||
assert.deepStrictEqual(Object.keys(ns), ['default']); | ||
assert.deepStrictEqual(ns.default, plainESMURL); | ||
} | ||
{ | ||
const body = '{"x": 1}'; | ||
const plainESMURL = createURL('application/json', body); | ||
const ns = await import(plainESMURL); | ||
assert.deepStrictEqual(Object.keys(ns), ['default']); | ||
assert.deepStrictEqual(ns.default.x, 1); | ||
} | ||
{ | ||
const body = '{"default": 2}'; | ||
const plainESMURL = createURL('application/json', body); | ||
const ns = await import(plainESMURL); | ||
assert.deepStrictEqual(Object.keys(ns), ['default']); | ||
assert.deepStrictEqual(ns.default.default, 2); | ||
} | ||
{ | ||
const body = 'null'; | ||
const plainESMURL = createURL('invalid', body); | ||
try { | ||
await import(plainESMURL); | ||
common.mustNotCall()(); | ||
} catch (e) { | ||
assert.strictEqual(e.code, 'ERR_INVALID_RETURN_PROPERTY_VALUE'); | ||
} | ||
} | ||
})().then(common.mustCall()); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text/javascript doesn't necessarily mean Module, it could also mean Script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under no JS loading spec does Script get checked against the MIME text/javascript. Script is effectively without a MIME and this table matches web standards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we support (browser-style) Script anywhere, so it would feel weird to do that here. A CommonJS script would be something like
text/vnd.node.js
according to nodejs/TSC#371.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CJS is application/node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then can
application/node
be added to this object?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expression though —
/^([^/]+\/[^;,]+)(;base64)?,/
seems to assume the presence of mime type before the[;,]
— regardless of if it is mandatory, it might make sense to test it against long and malformed urls to potentially refine it if necessary.Sorry for not wanting to muddy this with a bad attempt to wing it here, but I will try to locate the ones I worked on a while back for that very same purpose if it helps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SMotaal we could, but i can't think of much we could do except limiting the size? Right now this lacks a variety of things, including MIME parameter parsing and the PR for parsing MIMEs is stuck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it is anchored, it is certainly possible to add efficient guards in the current expression. I'd like to take on exploring how we can do that here, which is mainly just to carve a limited allowed chars when delimited per spec (I did this a while back just need to dig).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bmeck I looked into the various options for the expression and recommend:
/^(?:((?:text|application)\/(?:[A-Z][-.0-9A-Z]*)?[A-Z]+)((?:;[A-Z][!%'()*\-.0-9A-Z_~]*=[!%'()*\-.0-9A-Z_~]*)*)(;base64)?),/i
This would match any
text/
andapplication/
subtype, along with the attribute-value parameters likecharset=
(to be parsed separately), and optionalbase64
(captured separately from previous parameters).For now, simply being more restrictive of the character ranges for greedy
*
and+
captures is likely all we need to avoid unpredictable performance hazards with very long crafted/malformed strings.See gist for more details.
Please let me know how to proceed, if this is worth incorporating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just post a
suggestion
change and that looks fine