Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions lib/internal/modules/esm/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const {
ObjectValues,
} = primordials;
const { validateString } = require('internal/validators');
const { getOptionValue } = require('internal/options');

const {
ERR_IMPORT_ATTRIBUTE_TYPE_INCOMPATIBLE,
Expand All @@ -32,6 +33,14 @@ const formatTypeMap = {
'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
Comment on lines 32 to 34
Copy link
Copy Markdown
Member

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.

Suggested change
'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
'module': kImplicitTypeAttribute,
'text': 'text',
'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

Copy link
Copy Markdown
Contributor Author

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

};

function getFormatType(format) {
if (format === 'text' && getOptionValue('--experimental-import-text')) {
return 'text';
}

return formatTypeMap[format];
}

/**
* The HTML spec disallows the default type to be explicitly specified
* (for now); so `import './file.js'` is okay but
Expand All @@ -42,7 +51,6 @@ const supportedTypeAttributes = ArrayPrototypeFilter(
ObjectValues(formatTypeMap),
(type) => type !== kImplicitTypeAttribute);


/**
* Test a module's import attributes.
* @param {string} url The URL of the imported module, for error reporting.
Expand All @@ -60,7 +68,7 @@ function validateAttributes(url, format,
throw new ERR_IMPORT_ATTRIBUTE_UNSUPPORTED(keys[i], importAttributes[keys[i]], url);
}
}
const validType = formatTypeMap[format];
const validType = getFormatType(format);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

. . . and this change can be reverted . . .

Suggested change
const validType = getFormatType(format);
const validType = formatTypeMap[format];

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done


switch (validType) {
case undefined:
Expand Down Expand Up @@ -101,7 +109,8 @@ function handleInvalidType(url, type) {
validateString(type, 'type');

// `type` might not have been one of the types we understand.
if (!ArrayPrototypeIncludes(supportedTypeAttributes, type)) {
if (!ArrayPrototypeIncludes(supportedTypeAttributes, type) &&
!(type === 'text' && getOptionValue('--experimental-import-text'))) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

. . . and this change can be reverted . . .

Suggested change
if (!ArrayPrototypeIncludes(supportedTypeAttributes, type) &&
!(type === 'text' && getOptionValue('--experimental-import-text'))) {
if (!ArrayPrototypeIncludes(supportedTypeAttributes, type)) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

throw new ERR_IMPORT_ATTRIBUTE_UNSUPPORTED('type', type, url);
}

Expand Down
33 changes: 31 additions & 2 deletions lib/internal/modules/esm/get_format.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,25 @@ const protocolHandlers = {
'node:'() { return 'builtin'; },
};

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;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We already support with { type: 'json' }. This seems to introduce a new pattern, rather than following the existing one?

Copy link
Copy Markdown
Contributor Author

@efekrskl efekrskl Mar 29, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@GeoffreyBooth GeoffreyBooth Mar 29, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, thanks for the guidance.

I've updated the patch, wdyt?


/**
* Determine whether the given ambiguous source contains CommonJS or ES module syntax.
* @param {string | Buffer | undefined} [source]
Expand Down Expand Up @@ -238,10 +257,15 @@ function getFileProtocolModuleFormat(url, context = { __proto__: null }, ignoreE

/**
* @param {URL} url
* @param {{parentURL: string}} context
* @param {{parentURL: string, importAttributes?: Record<string, string>}} context
* @returns {Promise<string> | string | undefined} only works when enabled
*/
function defaultGetFormatWithoutErrors(url, context) {
const format = getFormatFromImportAttributes(context?.importAttributes);
if (format !== undefined) {
return format;
}

const protocol = url.protocol;
if (!ObjectPrototypeHasOwnProperty(protocolHandlers, protocol)) {
return null;
Expand All @@ -251,10 +275,15 @@ function defaultGetFormatWithoutErrors(url, context) {

/**
* @param {URL} url
* @param {{parentURL: string}} context
* @param {{parentURL: string, importAttributes?: Record<string, string>}} context
* @returns {Promise<string> | string | undefined} only works when enabled
*/
function defaultGetFormat(url, context) {
const format = getFormatFromImportAttributes(context?.importAttributes);
if (format !== undefined) {
return format;
}

const protocol = url.protocol;
if (!ObjectPrototypeHasOwnProperty(protocolHandlers, protocol)) {
return null;
Expand Down
10 changes: 8 additions & 2 deletions lib/internal/modules/esm/load.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,10 @@ function defaultLoad(url, context = kEmptyObject) {

if (format == null) {
// Now that we have the source for the module, run `defaultGetFormat` to detect its format.
format = defaultGetFormat(urlInstance, context);
format = defaultGetFormat(urlInstance, {
__proto__: context,
importAttributes,
});

if (format === 'commonjs') {
// For backward compatibility reasons, we need to discard the source in
Expand Down Expand Up @@ -155,7 +158,10 @@ function defaultLoadSync(url, context = kEmptyObject) {
}

// Now that we have the source for the module, run `defaultGetFormat` to detect its format.
format ??= defaultGetFormat(urlInstance, context);
format ??= defaultGetFormat(urlInstance, {
__proto__: context,
importAttributes,
});

// For backward compatibility reasons, we need to let go through Module._load
// again.
Expand Down
10 changes: 10 additions & 0 deletions lib/internal/modules/esm/translators.js
Original file line number Diff line number Diff line change
Expand Up @@ -642,3 +642,13 @@ 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) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

When strip-types was experimental, how were these handled? Were the translators just never set at all if the flag wasn’t passed, or would they throw within the function if the flag wasn’t passed, or something else?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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, I'm also adding a warning

let { source } = translateContext;
assertBufferSource(source, true, 'load');
source = stringify(source);
return new ModuleWrap(url, undefined, ['default'], function() {
this.setExport('default', source);
});
});
5 changes: 5 additions & 0 deletions src/node_options.cc
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,11 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() {
AddAlias("--loader", "--experimental-loader");
AddOption("--experimental-modules", "", NoOp{}, kAllowedInEnvvar);
AddOption("--experimental-wasm-modules", "", NoOp{}, kAllowedInEnvvar);
AddOption("--experimental-import-text",
"experimental support for importing source as text with import "
"attributes",
&EnvironmentOptions::experimental_import_text,
kAllowedInEnvvar);
AddOption("--experimental-import-meta-resolve",
"experimental ES Module import.meta.resolve() parentURL support",
&EnvironmentOptions::experimental_import_meta_resolve,
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ class EnvironmentOptions : public Options {
std::string localstorage_file;
bool experimental_global_navigator = true;
bool experimental_global_web_crypto = true;
bool experimental_import_text = false;
bool experimental_import_meta_resolve = false;
std::string input_type; // Value of --input-type
bool entry_is_url = false;
Expand Down
5 changes: 5 additions & 0 deletions test/es-module/test-esm-import-attributes-errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@ async function test() {
{ code: 'ERR_IMPORT_ATTRIBUTE_TYPE_INCOMPATIBLE' }
);

await assert.rejects(
import(jsModuleDataUrl, { with: { type: 'text' } }),
{ code: 'ERR_IMPORT_ATTRIBUTE_UNSUPPORTED' }
);

await assert.rejects(
import(jsModuleDataUrl, { with: { type: 'json', other: 'unsupported' } }),
{ code: 'ERR_IMPORT_ATTRIBUTE_UNSUPPORTED' }
Expand Down
5 changes: 5 additions & 0 deletions test/es-module/test-esm-import-attributes-errors.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ await assert.rejects(
{ code: 'ERR_IMPORT_ATTRIBUTE_TYPE_INCOMPATIBLE' }
);

await assert.rejects(
import(jsModuleDataUrl, { with: { type: 'text' } }),
{ code: 'ERR_IMPORT_ATTRIBUTE_UNSUPPORTED' }
);

await assert.rejects(
import(jsModuleDataUrl, { with: { type: 'json', other: 'unsupported' } }),
{ code: 'ERR_IMPORT_ATTRIBUTE_UNSUPPORTED' }
Expand Down
19 changes: 19 additions & 0 deletions test/es-module/test-esm-import-attributes-validation-text.js
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',
});
12 changes: 12 additions & 0 deletions test/es-module/test-esm-import-text-disabled.mjs
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' },
);
51 changes: 51 additions & 0 deletions test/es-module/test-esm-import-text.mjs
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_UNKNOWN_MODULE_FORMAT' },
);

await assert.rejects(
import('../fixtures/file-to-read-without-bom.txt'),
{ code: 'ERR_UNKNOWN_FILE_EXTENSION' },
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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 image.png or mod.wasm.

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" };

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Replacement characters render the text form of a non-text file unusable and unrecoverable, so it's hard to think of a use case.

And implementing a list of potential extensions which don't make sense could be a never ending process?

I don't suggest filtering by extension. I suggest throwing an exception if any non-text byte is encountered when reading the file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Which bytes are always, universally, non-text?

Copy link
Copy Markdown
Contributor

@bakkot bakkot Apr 8, 2026

Choose a reason for hiding this comment

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

This loader unconditionally uses UTF-8 encoding, so presumably "non-text" means "not valid UTF-8", which means this amounts to a request to throw instead of substituting � U+FFFD when bytes which are not valid UTF-8 are encountered (i.e. passing fatal: true to the TextDecoder used to decode the bytes).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The 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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Loading