-
-
Notifications
You must be signed in to change notification settings - Fork 35.3k
loader: add experimental text import #62300
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
base: main
Are you sure you want to change the base?
Changes from all commits
66a1503
a392f74
2da51d1
7fd27d8
5c3addd
63428bb
4a5953c
f8de5d5
42428d9
4a29525
31778cd
deb71df
4c1debd
b9fcd98
4c18c68
e92f71a
d04ff90
5ecccba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -642,3 +642,14 @@ translators.set('module-typescript', function(url, translateContext, parentURL) | |
| translateContext.source = stripTypeScriptModuleTypes(stringify(source), url); | ||
| return FunctionPrototypeCall(translators.get('module'), this, url, translateContext, parentURL); | ||
| }); | ||
|
|
||
| // Strategy for loading source as text. | ||
| translators.set('text', function textStrategy(url, translateContext) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like they were introduced in 35f92d9 and they were never gated, since it would fail earlier I see now they emitted a warning with |
||
| emitExperimentalWarning('Text import'); | ||
| let { source } = translateContext; | ||
| assertBufferSource(source, true, 'load'); | ||
| source = stringify(source); | ||
| return new ModuleWrap(url, undefined, ['default'], function() { | ||
| this.setExport('default', source); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| // Flags: --expose-internals --experimental-import-text | ||
| 'use strict'; | ||
| require('../common'); | ||
|
|
||
| const assert = require('assert'); | ||
|
|
||
| const { validateAttributes } = require('internal/modules/esm/assert'); | ||
|
|
||
| const url = 'test://'; | ||
|
|
||
| assert.ok(validateAttributes(url, 'text', { type: 'text' })); | ||
|
|
||
| assert.throws(() => validateAttributes(url, 'text', {}), { | ||
| code: 'ERR_IMPORT_ATTRIBUTE_MISSING', | ||
| }); | ||
|
|
||
| assert.throws(() => validateAttributes(url, 'module', { type: 'text' }), { | ||
| code: 'ERR_IMPORT_ATTRIBUTE_TYPE_INCOMPATIBLE', | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| import '../common/index.mjs'; | ||
| import assert from 'assert'; | ||
|
|
||
| await assert.rejects( | ||
| import('../fixtures/file-to-read-without-bom.txt', { with: { type: 'text' } }), | ||
| { code: 'ERR_UNKNOWN_FILE_EXTENSION' }, | ||
| ); | ||
|
|
||
| await assert.rejects( | ||
| import('data:text/plain,hello%20world', { with: { type: 'text' } }), | ||
| { code: 'ERR_UNKNOWN_MODULE_FORMAT' }, | ||
| ); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| // Flags: --experimental-import-text | ||
| import '../common/index.mjs'; | ||
| import assert from 'assert'; | ||
|
|
||
| import staticText from '../fixtures/file-to-read-without-bom.txt' with { type: 'text' }; | ||
| import staticTextWithBOM from '../fixtures/file-to-read-with-bom.txt' with { type: 'text' }; | ||
|
|
||
| const expectedText = 'abc\ndef\nghi\n'; | ||
|
|
||
| assert.strictEqual(staticText, expectedText); | ||
| assert.strictEqual(staticTextWithBOM, expectedText); | ||
|
|
||
| const dynamicText = await import('../fixtures/file-to-read-without-bom.txt', { | ||
| with: { type: 'text' }, | ||
| }); | ||
| assert.strictEqual(dynamicText.default, expectedText); | ||
|
|
||
| const dataText = await import('data:text/plain,hello%20world', { | ||
| with: { type: 'text' }, | ||
| }); | ||
| assert.strictEqual(dataText.default, 'hello world'); | ||
|
|
||
| const dataJsAsText = await import('data:text/javascript,export{}', { | ||
| with: { type: 'text' }, | ||
| }); | ||
| assert.strictEqual(dataJsAsText.default, 'export{}'); | ||
|
|
||
| const dataInvalidUtf8 = await import('data:text/plain,%66%6f%80%6f', { | ||
| with: { type: 'text' }, | ||
| }); | ||
| assert.strictEqual(dataInvalidUtf8.default, 'fo\ufffdo'); | ||
|
|
||
| const jsAsText = await import('../fixtures/syntax/bad_syntax.js', { | ||
| with: { type: 'text' }, | ||
| }); | ||
| assert.match(jsAsText.default, /^var foo bar;/); | ||
|
|
||
| const jsonAsText = await import('../fixtures/invalid.json', { | ||
| with: { type: 'text' }, | ||
| }); | ||
| assert.match(jsonAsText.default, /"im broken"/); | ||
|
|
||
| await assert.rejects( | ||
| import('data:text/plain,hello%20world'), | ||
| { code: 'ERR_IMPORT_ATTRIBUTE_MISSING' }, | ||
| ); | ||
|
|
||
| await assert.rejects( | ||
| import('../fixtures/file-to-read-without-bom.txt'), | ||
| { code: 'ERR_UNKNOWN_FILE_EXTENSION' }, | ||
| ); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a test for what happens when you try to import a binary data file such as an I encourage Node.js to take a strict view here. The following code should throw an exception import text from "image.png" with { type: "text" };There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'm not so sure. I would just allow any file to be read as text. You never know it might have a use case. And implementing a list of potential extensions which don't make sense could be a never ending process?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm really not sure why it should - if there's a text representation of a file, it should give it. If it's nonsense, that's the developer's problem to solve. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Replacement characters render the text form of a non-text file unusable and unrecoverable, so it's hard to think of a use case.
I don't suggest filtering by extension. I suggest throwing an exception if any non-text byte is encountered when reading the file.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which bytes are always, universally, non-text?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but valid javascript strings can have lone surrogates, so i'm a bit confused why we'd want to do either one - meaning, throwing kills use cases, and silently substituting results in artificially well-formed utf8? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When reading files, it's always necessary to push them through a text decoder, except the unusual case where the file is stored on disk as UTF-16. There is no such thing as lone surrogates in UTF-8.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit unsure about throwing here. Neither Deno nor Bun throw on invalid UTF-8 in this case, so I'm leaning toward keeping Node aligned. Of course, I'm open to being challenged if there's a strong reason. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do Deno and Bun ship this feature? It is not a standard feature yet; it is moving through committee. Node should do what's right, not what implementations of a non-standard feature have done.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unflagged in Bun since April 2024, and flagged in Deno since June 2025. That doesn't necessarily mean anyone's depending on being able to import invalid UTF-8 with replacement characters instead of an error, though. |
||
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 this map is never exported, I think it can have experimental formats? Then we won’t need to override it or check in so many places.
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.
Yup, we can extend this directly, done