-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
Changes from 4 commits
887ee9b
fcb8be9
bfaedc4
500ea04
a82e860
92747ab
d054cd0
a177750
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 |
---|---|---|
|
@@ -7,6 +7,9 @@ | |
<!-- YAML | ||
added: v8.5.0 | ||
changes: | ||
- version: REPLACEME | ||
pr-url: https://github.com/nodejs/node/pull/49531 | ||
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. | ||
|
@@ -156,15 +159,6 @@ via the paths defined in [`"exports"`][]. | |
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. | ||
|
||
### URLs | ||
|
||
ES modules are resolved and cached as URLs. This means that special characters | ||
|
@@ -1008,7 +1002,7 @@ _isImports_, _conditions_) | |
> 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 | ||
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. 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. 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 elaborate if you mean a need to reword this or suggest changes to the implementation. 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 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, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,6 @@ const { | |
StringPrototypeCharCodeAt, | ||
StringPrototypeSlice, | ||
} = primordials; | ||
const { basename, relative } = require('path'); | ||
const { getOptionValue } = require('internal/options'); | ||
const { | ||
extensionFormatMap, | ||
|
@@ -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; | ||
|
||
|
@@ -74,7 +73,7 @@ function extname(url) { | |
*/ | ||
function getFileProtocolModuleFormat(url, context, ignoreErrors) { | ||
const ext = extname(url); | ||
if (ext === '.js') { | ||
if (ext === '.js' || ext === '') { | ||
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 think we’re aiming to change how extensionless files are handled as entry points, not in general from any 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. How come? Both ESM with other protocols and CJS have no problems with extensionless imports. 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 would be very weird to have a file parsed as ESM only some of the time, I much prefer the current approach. 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. In the original implementation before it was limited, we supported extensionless per 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.
In the comment you just linked, you say:
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. 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)
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 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 But I think we can support |
||
return getPackageType(url) === 'module' ? 'module' : 'commonjs'; | ||
} | ||
|
||
|
@@ -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); | ||
} | ||
|
||
/** | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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'); | ||
})); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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'); | ||
})); | ||
} |
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.
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.