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

module: support ES modules without file extension within module scope #49531

Closed
wants to merge 8 commits into from
14 changes: 4 additions & 10 deletions doc/api/esm.md
Expand Up @@ -7,6 +7,9 @@
<!-- YAML
added: v8.5.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/49531

Check warning on line 11 in doc/api/esm.md

View workflow job for this annotation

GitHub Actions / lint-pr-url

pr-url doesn't match the URL of the current PR.
description: ES modules within `module` package can be extensionless.
- version: v20.0.0
pr-url: https://github.com/nodejs/node/pull/44710
description: Module customization hooks are executed off the main thread.
Expand Down Expand Up @@ -156,15 +159,6 @@
For details on these package resolution rules that apply to bare specifiers in
the Node.js module resolution, see the [packages documentation](packages.md).

### Mandatory file extensions

A file extension must be provided when using the `import` keyword to resolve
relative or absolute specifiers. Directory indexes (e.g. `'./startup/index.js'`)
must also be fully specified.

This behavior matches how `import` behaves in browser environments, assuming a
typically configured server.
Copy link
Member

Choose a reason for hiding this comment

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

This is referring to import, not entry points, and shouldn’t be removed. I think the docs for this PR would more or less match the reverse of the changes in #31415.


### URLs

ES modules are resolved and cached as URLs. This means that special characters
Expand Down Expand Up @@ -1008,7 +1002,7 @@
> 5. Let _packageURL_ be the result of **LOOKUP\_PACKAGE\_SCOPE**(_url_).
> 6. Let _pjson_ be the result of **READ\_PACKAGE\_JSON**(_packageURL_).
> 7. If _pjson?.type_ exists and is _"module"_, then
> 1. If _url_ ends in _".js"_, then
> 1. If _url_ ends in _".js"_ or lacks file extension, then
Copy link
Member

Choose a reason for hiding this comment

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

type module controls .js, but there’s ways to use ESM without type module - it shouldn’t be overloaded to also control extensionless, and there needs to be a way to use extensionless ESM without type module imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please elaborate if you mean a need to reword this or suggest changes to the implementation.
The type controls how any JS code is interpreted. The exceptions are files with explicitly typed extensions (.cjs, .mjs, maybe future formats) and with unknown extensions (these should throw to ensure that it breakage is minimized if we add future formats).

Copy link
Member

Choose a reason for hiding this comment

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

I’m suggesting an additional package.json key to control extensionless. “type” only controls how .js is interpreted, not “any” js code.

> 1. Return _"module"_.
> 2. Return **undefined**.
> 8. Otherwise,
Expand Down
4 changes: 1 addition & 3 deletions doc/api/packages.md
Expand Up @@ -393,8 +393,7 @@ and other JavaScript runtimes, using the extensionless style can result in
bloated import map definitions. Explicit file extensions can avoid this issue by
enabling the import map to utilize a [packages folder mapping][] to map multiple
subpaths where possible instead of a separate map entry per package subpath
export. This also mirrors the requirement of using [the full specifier path][]
in relative and absolute import specifiers.
export.

### Exports sugar

Expand Down Expand Up @@ -1352,4 +1351,3 @@ This field defines [subpath imports][] for the current package.
[subpath imports]: #subpath-imports
[supported package managers]: corepack.md#supported-package-managers
[the dual CommonJS/ES module packages section]: #dual-commonjses-module-packages
[the full specifier path]: esm.md#mandatory-file-extensions
8 changes: 1 addition & 7 deletions lib/internal/errors.js
Expand Up @@ -1690,13 +1690,7 @@ E('ERR_UNHANDLED_ERROR',
E('ERR_UNKNOWN_BUILTIN_MODULE', 'No such built-in module: %s', Error);
E('ERR_UNKNOWN_CREDENTIAL', '%s identifier does not exist: %s', Error);
E('ERR_UNKNOWN_ENCODING', 'Unknown encoding: %s', TypeError);
E('ERR_UNKNOWN_FILE_EXTENSION', (ext, path, suggestion) => {
let msg = `Unknown file extension "${ext}" for ${path}`;
if (suggestion) {
msg += `. ${suggestion}`;
}
return msg;
}, TypeError);
E('ERR_UNKNOWN_FILE_EXTENSION', 'Unknown file extension %s for %s', TypeError);
E('ERR_UNKNOWN_MODULE_FORMAT', 'Unknown module format: %s for URL %s',
RangeError);
E('ERR_UNKNOWN_SIGNAL', 'Unknown signal: %s', TypeError);
Expand Down
20 changes: 4 additions & 16 deletions lib/internal/modules/esm/get_format.js
Expand Up @@ -7,7 +7,6 @@ const {
StringPrototypeCharCodeAt,
StringPrototypeSlice,
} = primordials;
const { basename, relative } = require('path');
const { getOptionValue } = require('internal/options');
const {
extensionFormatMap,
Expand All @@ -16,7 +15,7 @@ const {

const experimentalNetworkImports =
getOptionValue('--experimental-network-imports');
const { getPackageType, getPackageScopeConfig } = require('internal/modules/esm/resolve');
const { getPackageType } = require('internal/modules/esm/resolve');
const { fileURLToPath } = require('internal/url');
const { ERR_UNKNOWN_FILE_EXTENSION } = require('internal/errors').codes;

Expand Down Expand Up @@ -74,7 +73,7 @@ function extname(url) {
*/
function getFileProtocolModuleFormat(url, context, ignoreErrors) {
const ext = extname(url);
if (ext === '.js') {
if (ext === '.js' || ext === '') {
Copy link
Member

Choose a reason for hiding this comment

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

I think we’re aiming to change how extensionless files are handled as entry points, not in general from any import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How come? Both ESM with other protocols and CJS have no problems with extensionless imports.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be very weird to have a file parsed as ESM only some of the time, I much prefer the current approach.

Copy link
Member

Choose a reason for hiding this comment

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

In the original implementation before it was limited, we supported extensionless per "type" only for entry points, not for any import, per #31415 (comment). We’ve never supported extensionless for import. I could search around for why there was the original limitation to make this only for entry points, but I’d bet there was a good reason. There’s also not really a use case for import of extensionless files.

Copy link
Contributor

Choose a reason for hiding this comment

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

We’ve never supported extensionless for import

In the comment you just linked, you say:

support for extensionless files in import was added only last month, in #31021.

Copy link
Member

Choose a reason for hiding this comment

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

Okay so we didn’t never support it, but it wasn’t part of the original implementation. It was briefly added and then reverted. Probably because of this: #31021 (review)

One of the major features of the ES module resolver is being able to support new file extensions in future, which is reliant on the fact that we always throw for unknown file extensions.

Giving special treatment to isMain was how we did this while ensuring compatibility with existing bins.

Happy to flesh this out further, but I don’t think we should lose that extension property for the ESM resolver.

Copy link
Member

Choose a reason for hiding this comment

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

I dug through a lot of old issues and consulted @guybedford to try to rediscover why we limited extensionless support to entry points in the initial implementation, and why we removed even that in #31415. Basically, we wanted to preserve the ability for future extensionless Wasm entry points. And I think we still do; that goal hasn’t changed, and #49540 is a viable way to achieve that goal. I think years ago we weren’t aware of or really thinking about the “disambiguate based on magic bytes” solution, and so we just locked down the design space in the hope that we’d figure something out eventually. And I think the magic bytes approach is that something, and so it can open up the ability for both extensionless ESM entry points and extensionless Wasm entry points.

As for import of extensionless files, there would be the similar question of how to disambiguate an import of an extensionless JavaScript file from an extensionless Wasm file. The simplest solution is probably to just take same approach that we use for entry points, looking at magic bytes. Currently import of Wasm is gated behind the separate flag --experimental-wasm-modules, so it doesn’t matter all that much.

But I think we can support import of extensionless JavaScript. JavaScript can be the default for an import statement, where “extensionless = JavaScript with the format determined by type” for import like for entry point, with other potential formats needing a way to disambiguate. Those new types would either need explicit extensions (the most likely and obvious disambiguation) or they would need mandatory import attributes (like JSON) or they would need magic bytes; some way to disambiguate them from JavaScript. But I think we can unlock import of extensionless JavaScript without much risk of that prohibiting future enhancements.

return getPackageType(url) === 'module' ? 'module' : 'commonjs';
}

Expand All @@ -83,20 +82,9 @@ function getFileProtocolModuleFormat(url, context, ignoreErrors) {

// Explicit undefined return indicates load hook should rerun format check
if (ignoreErrors) { return undefined; }

const filepath = fileURLToPath(url);
let suggestion = '';
if (getPackageType(url) === 'module' && ext === '') {
const config = getPackageScopeConfig(url);
const fileBasename = basename(filepath);
const relativePath = StringPrototypeSlice(relative(config.pjsonPath, filepath), 1);
suggestion = 'Loading extensionless files is not supported inside of ' +
'"type":"module" package.json contexts. The package.json file ' +
`${config.pjsonPath} caused this "type":"module" context. Try ` +
`changing ${filepath} to have a file extension. Note the "bin" ` +
'field of package.json can point to a file with an extension, for example ' +
`{"type":"module","bin":{"${fileBasename}":"${relativePath}.js"}}`;
}
throw new ERR_UNKNOWN_FILE_EXTENSION(ext, filepath, suggestion);
throw new ERR_UNKNOWN_FILE_EXTENSION(ext, filepath);
}

/**
Expand Down
36 changes: 0 additions & 36 deletions test/es-module/test-esm-unknown-or-no-extension.js

This file was deleted.

22 changes: 22 additions & 0 deletions test/parallel/test-esm-no-extension.mjs
@@ -0,0 +1,22 @@
import * as common from '../common/index.mjs';
import * as fixtures from '../common/fixtures.mjs';
import { spawn } from 'node:child_process';
import assert from 'node:assert';

const entry = fixtures.path('/es-modules/package-type-module/noext-esm');

// Run a module that does not have extension.
// This is to ensure that "type": "module" applies to extensionless files.

const child = spawn(process.execPath, [entry]);

let stdout = '';
child.stdout.setEncoding('utf8');
child.stdout.on('data', (data) => {
stdout += data;
});
child.on('close', common.mustCall((code, signal) => {
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
assert.strictEqual(stdout, 'executed\n');
}));
79 changes: 79 additions & 0 deletions test/parallel/test-esm-unknown-main.mjs
@@ -0,0 +1,79 @@
import * as common from '../common/index.mjs';
import * as fixtures from '../common/fixtures.mjs';
import { spawn } from 'node:child_process';
import assert from 'node:assert';

{
const entry = fixtures.path(
'/es-modules/package-type-module/extension.unknown'
);
const child = spawn(process.execPath, [entry]);
let stdout = '';
let stderr = '';
child.stderr.setEncoding('utf8');
child.stdout.setEncoding('utf8');
child.stdout.on('data', (data) => {
stdout += data;
});
child.stderr.on('data', (data) => {
stderr += data;
});
child.on('close', common.mustCall((code, signal) => {
assert.strictEqual(code, 1);
assert.strictEqual(signal, null);
assert.strictEqual(stdout, '');
assert.ok(stderr.indexOf('ERR_UNKNOWN_FILE_EXTENSION') !== -1);
}));
}
{
const entry = fixtures.path(
'/es-modules/package-type-module/imports-unknownext.mjs'
);
const child = spawn(process.execPath, [entry]);
let stdout = '';
let stderr = '';
child.stderr.setEncoding('utf8');
child.stdout.setEncoding('utf8');
child.stdout.on('data', (data) => {
stdout += data;
});
child.stderr.on('data', (data) => {
stderr += data;
});
child.on('close', common.mustCall((code, signal) => {
assert.strictEqual(code, 1);
assert.strictEqual(signal, null);
assert.strictEqual(stdout, '');
assert.ok(stderr.indexOf('ERR_UNKNOWN_FILE_EXTENSION') !== -1);
}));
}
{
const entry = fixtures.path('/es-modules/package-type-module/noext-esm');
const child = spawn(process.execPath, [entry]);
let stdout = '';
child.stdout.setEncoding('utf8');
child.stdout.on('data', (data) => {
stdout += data;
});
child.on('close', common.mustCall((code, signal) => {
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
assert.strictEqual(stdout, 'executed\n');
}));
}
{
const entry = fixtures.path(
'/es-modules/package-type-module/imports-noext.mjs'
);
const child = spawn(process.execPath, [entry]);
let stdout = '';
child.stdout.setEncoding('utf8');
child.stdout.on('data', (data) => {
stdout += data;
});
child.on('close', common.mustCall((code, signal) => {
assert.strictEqual(code, 0);
assert.strictEqual(signal, null);
assert.strictEqual(stdout, 'executed\n');
}));
}