-
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
esm: make specifier flag clearly experimental #30678
Changes from all commits
fb2d670
60a9dcb
db9f3ec
f4eb371
118171c
7bf301e
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 |
---|---|---|
|
@@ -13,8 +13,8 @@ const { getOptionValue } = require('internal/options'); | |
const preserveSymlinks = getOptionValue('--preserve-symlinks'); | ||
const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); | ||
const experimentalJsonModules = getOptionValue('--experimental-json-modules'); | ||
const esModuleSpecifierResolution = | ||
getOptionValue('--es-module-specifier-resolution'); | ||
const experimentalSpeciferResolution = | ||
getOptionValue('--experimental-specifier-resolution'); | ||
const typeFlag = getOptionValue('--input-type'); | ||
const experimentalWasmModules = getOptionValue('--experimental-wasm-modules'); | ||
const { resolve: moduleWrapResolve, | ||
|
@@ -110,10 +110,14 @@ function resolve(specifier, parentURL) { | |
if (ext === '.js' || (!format && isMain)) | ||
format = getPackageType(url.href) === TYPE_MODULE ? 'module' : 'commonjs'; | ||
if (!format) { | ||
if (esModuleSpecifierResolution === 'node') | ||
if (experimentalSpeciferResolution === 'node') { | ||
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. should we emit the warning on the use of the flag or only when it is active? 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 tend to prefer the pattern in general of having the warning on first use of the feature. That's what we're looking at doing for the other experimental modules features. 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. The only problem is this might not catch all usage though - because we have branches in the C++ resolver which check this which may not still catch this path (this path is specifically for non JS file extensions). So we should either bring the warning to the C++ code, or we should make it a general warning on startup. 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. 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. Is getting the warning into C++ layer blocking this from landing or should we do it in a follow up? |
||
process.emitWarning( | ||
'The Node.js specifier resolution in ESM is experimental.', | ||
'ExperimentalWarning'); | ||
format = legacyExtensionFormatMap[ext]; | ||
else | ||
} else { | ||
throw new ERR_UNKNOWN_FILE_EXTENSION(fileURLToPath(url)); | ||
} | ||
} | ||
return { url: `${url}`, format }; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,9 +128,23 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) { | |
} | ||
|
||
if (!es_module_specifier_resolution.empty()) { | ||
if (es_module_specifier_resolution != "node" && | ||
es_module_specifier_resolution != "explicit") { | ||
errors->push_back("invalid value for --es-module-specifier-resolution"); | ||
if (!experimental_specifier_resolution.empty()) { | ||
MylesBorins marked this conversation as resolved.
Show resolved
Hide resolved
|
||
errors->push_back( | ||
"bad option: cannot use --es-module-specifier-resolution" | ||
" and --experimental-specifier-resolution at the same time"); | ||
} else { | ||
experimental_specifier_resolution = es_module_specifier_resolution; | ||
if (experimental_specifier_resolution != "node" && | ||
experimental_specifier_resolution != "explicit") { | ||
errors->push_back( | ||
"invalid value for --es-module-specifier-resolution"); | ||
} | ||
} | ||
} else if (!experimental_specifier_resolution.empty()) { | ||
if (experimental_specifier_resolution != "node" && | ||
experimental_specifier_resolution != "explicit") { | ||
errors->push_back( | ||
"invalid value for --experimental-specifier-resolution"); | ||
} | ||
} | ||
|
||
|
@@ -361,9 +375,13 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { | |
"set module type for string input", | ||
&EnvironmentOptions::module_type, | ||
kAllowedInEnvironment); | ||
AddOption("--es-module-specifier-resolution", | ||
AddOption("--experimental-specifier-resolution", | ||
"Select extension resolution algorithm for es modules; " | ||
"either 'explicit' (default) or 'node'", | ||
&EnvironmentOptions::experimental_specifier_resolution, | ||
kAllowedInEnvironment); | ||
AddOption("--es-module-specifier-resolution", | ||
"", | ||
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. Fun fact, leaving no description on a flag stop it from printing when you run 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. We should probably adopt the same for the aliasing of 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. If this is an alias, shouldn't it be using 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 originally implemented it that way but couldn't figure out how to enforce behavior that both the alias and the actual command couldn't be used at the same time Ended up manually implemented this here. There was weird behavior depending on the order of arguments, so I opted to do this instead of hunting down how to get the same behavior as an alias. Is there a better way to do it? 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.
¯\_(ツ)_/¯ cc @addaleax 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 guess we could add a special kind of alias for this, but … why? 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. Alternatively, I think this could use at least 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. If y'all are happy with just shipping this as is, it is a one off and something that will eventually get removed. |
||
&EnvironmentOptions::es_module_specifier_resolution, | ||
kAllowedInEnvironment); | ||
AddOption("--no-deprecation", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import { mustCall } from '../common/index.mjs'; | ||
import { exec } from 'child_process'; | ||
import assert from 'assert'; | ||
|
||
const expectedError = | ||
'cannot use --es-module-specifier-resolution ' + | ||
'and --experimental-specifier-resolution at the same time'; | ||
|
||
const flags = '--es-module-specifier-resolution=node ' + | ||
'--experimental-specifier-resolution=node'; | ||
|
||
exec(`${process.execPath} ${flags}`, { | ||
timeout: 300 | ||
}, mustCall((error) => { | ||
assert(error.message.includes(expectedError)); | ||
MylesBorins marked this conversation as resolved.
Show resolved
Hide resolved
guybedford marked this conversation as resolved.
Show resolved
Hide resolved
|
||
})); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// Flags: --es-module-specifier-resolution=node | ||
import '../common/index.mjs'; | ||
import assert from 'assert'; | ||
|
||
// commonJS index.js | ||
import commonjs from '../fixtures/es-module-specifiers/package-type-commonjs'; | ||
// esm index.js | ||
import module from '../fixtures/es-module-specifiers/package-type-module'; | ||
// Notice the trailing slash | ||
import success, { explicit, implicit, implicitModule } | ||
from '../fixtures/es-module-specifiers/'; | ||
|
||
assert.strictEqual(commonjs, 'commonjs'); | ||
assert.strictEqual(module, 'module'); | ||
assert.strictEqual(success, 'success'); | ||
assert.strictEqual(explicit, 'esm'); | ||
assert.strictEqual(implicit, 'cjs'); | ||
assert.strictEqual(implicitModule, 'cjs'); |
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.
If we're changing the name I wonder if it might make sense to change the name of the extension searching resolution to
--experimental-specifier-resolution=legacy
or--experimental-specifier-resolution=compat
or something like that?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.
Maybe
--experimental-specifier-resolution=require
to make it obvious what it is emulating?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.
I'm pretty sure there was some distaste towards the implication that the flag was for legacy compatibility - the flag exists to test an alternative that could be a viable way forward.
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.
Would
--experimental-specifier-resolution=detect
capture the semantics?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.
I'm happy to go with what we have though, and have approved the PR.
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.
require
orcommonjs
ordetect
all would be acceptable to me;legacy
would not. (leaving it as "node" is also fine ofc)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.
errr, if you want a new name, why not
implicit
to mirrorexplicit
?This comment was marked as resolved.
Sorry, something went wrong.