Conversation
|
Review requested:
|
|
Note that the import-bytes proposal requires producing Uint8Arrays backed by immutable ArrayBuffers, which are not yet supported in V8. I would be hesitant to ship this with mutable backing buffers, even flagged as experimental, lest people come to rely on that. Text imports don't have the same problem. |
That's a great point, looks like I've missed that. I've removed |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62300 +/- ##
==========================================
+ Coverage 89.68% 89.70% +0.01%
==========================================
Files 676 692 +16
Lines 206578 214212 +7634
Branches 39555 41123 +1568
==========================================
+ Hits 185267 192154 +6887
- Misses 13446 14112 +666
- Partials 7865 7946 +81
🚀 New features to boost your workflow:
|
This is really good knowledge that would be good to preserve somewhere. Maybe add a comment near the most appropriate spot in the new code, such as where the |
| const formatTypeMap = { | ||
| '__proto__': null, | ||
| 'builtin': kImplicitTypeAttribute, | ||
| 'commonjs': kImplicitTypeAttribute, | ||
| 'json': 'json', | ||
| 'module': kImplicitTypeAttribute, | ||
| 'wasm': kImplicitTypeAttribute, // It's unclear whether the HTML spec will require an type attribute or not for Wasm; see https://github.com/WebAssembly/esm-integration/issues/42 | ||
| }; | ||
|
|
||
| function getFormatType(format) { | ||
| if (format === 'text' && getOptionValue('--experimental-import-text')) { | ||
| return 'text'; | ||
| } | ||
|
|
||
| return formatTypeMap[format]; | ||
| } |
There was a problem hiding this comment.
In the past when we've added experimental types, we conditionally add them to the map. GitHub has a bug that won't let me suggest a change, but basically (psuedocode):
formatTypeMap = { ... }
if (getOptionValue('--experimental-import-text') {
formatTypeMap.text = 'text';
}
Then you don't need the other changes farther down in this file.
There was a problem hiding this comment.
I don't think this particular example would work as we can't call getOptionValue at module top level during bootstrap. Maybe we could do it lazily when validateAttributes is called but then both formatTypeMap and supportedTypeAttributes has to be patched.
I'm not very happy about this part of the change, but looking into old commits couldn't come up with something better
| function getFormatFromImportAttributes(importAttributes) { | ||
| if ( | ||
| !importAttributes || | ||
| !ObjectPrototypeHasOwnProperty(importAttributes, 'type') || | ||
| typeof importAttributes.type !== 'string' | ||
| ) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if ( | ||
| getOptionValue('--experimental-import-text') && | ||
| importAttributes.type === 'text' | ||
| ) { | ||
| return 'text'; | ||
| } | ||
|
|
||
| return undefined; | ||
| } |
There was a problem hiding this comment.
We already support with { type: 'json' }. This seems to introduce a new pattern, rather than following the existing one?
There was a problem hiding this comment.
Good catch, the reason is JSON imports are format led (.json/MIME), whereas for text imports anything can be imported as text (regardless of .txt/MIME). So we needed to have a separate mapping
I'm open for ideas though. Maybe we can make this more clear with a comment
There was a problem hiding this comment.
Agreed that it should have a new mapping, but in general we want to follow the existing patterns of the cocebase. Don’t create a new code path for handling modules of varying formats; we already have a path for with { type: 'json' } so text should work the same way, only diverging where it needs to because of the type. For example, JSON didn’t require getFormatFromImportAttributes so this function shouldn’t be needed to add support for text.
There was a problem hiding this comment.
Makes sense, thanks for the guidance.
I've updated the patch, wdyt?
pdeveltere
left a comment
There was a problem hiding this comment.
in loader.js, you can add:
/**
* @typedef {'builtin'|'commonjs'|'json'|'module'|'text'|'wasm'} ModuleFormat
*/
| function getFileProtocolModuleFormat(url, context = { __proto__: null }, ignoreErrors) { | ||
| const { source } = context; | ||
| const ext = extname(url); | ||
|
|
There was a problem hiding this comment.
| // If the caller explicitly requests text format via import attributes, honour it regardless of file extension. | |
| if (context.importAttributes?.type === 'text') { | |
| return 'text'; | |
| } |
| ) !== null | ||
| ) { return 'module'; } | ||
| if (mime === 'application/json') { return 'json'; } | ||
| if (mime === 'application/wasm') { return 'wasm'; } |
There was a problem hiding this comment.
| if (mime === 'application/wasm') { return 'wasm'; } | |
| if (RegExpPrototypeExec(/^\s*text\/plain\s*(;\s*charset=utf-?8\s*)?$/i, mime) !== null) { return 'text'; } |
Yeah makes sense, I've added a comment right below |
Closes #62082
Adds support for text import under
--experimental-import-textflag.Example:
Import text is a Stage 3 TC39 proposal
Deno supports text imports with
--unstable-raw-imports.Bun supports text imports directly.
Not entirely sure what the current stance is on not-yet-landed proposals, but putting this up for discussion and feedback.