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

esm: make specifier flag clearly experimental #30678

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 15 additions & 15 deletions doc/api/cli.md
Expand Up @@ -156,20 +156,6 @@ Enable experimental Source Map V3 support for stack traces.
Currently, overriding `Error.prepareStackTrace` is ignored when the
`--enable-source-maps` flag is set.

### `--es-module-specifier-resolution=mode`
<!-- YAML
added: v12.0.0
-->

Sets the resolution algorithm for resolving ES module specifiers. Valid options
are `explicit` and `node`.

The default is `explicit`, which requires providing the full path to a
module. The `node` mode will enable support for optional file extensions and
the ability to import a directory that has an index file.

Please see [customizing ESM specifier resolution][] for example usage.

### `--experimental-conditional-exports`
<!-- YAML
added: v13.2.0
Expand Down Expand Up @@ -223,6 +209,20 @@ added: v13.1.0
Enable experimental support for a package using `require` or `import` to load
itself.

### `--experimental-specifier-resolution=mode`
<!-- YAML
added: REPLACEME
-->

Sets the resolution algorithm for resolving ES module specifiers. Valid options
are `explicit` and `node`.
Copy link
Contributor

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?

Copy link
Contributor

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?

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.

Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Member

@ljharb ljharb Nov 27, 2019

Choose a reason for hiding this comment

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

require or commonjs or detect all would be acceptable to me; legacy would not. (leaving it as "node" is also fine ofc)

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 mirror explicit?

This comment was marked as resolved.


The default is `explicit`, which requires providing the full path to a
module. The `node` mode will enable support for optional file extensions and
the ability to import a directory that has an index file.

Please see [customizing ESM specifier resolution][] for example usage.

### `--experimental-vm-modules`
<!-- YAML
added: v9.6.0
Expand Down Expand Up @@ -1038,7 +1038,6 @@ Node.js options that are allowed are:
<!-- node-options-node start -->
* `--enable-fips`
* `--enable-source-maps`
* `--es-module-specifier-resolution`
* `--experimental-conditional-exports`
* `--experimental-json-modules`
* `--experimental-loader`
Expand All @@ -1047,6 +1046,7 @@ Node.js options that are allowed are:
* `--experimental-repl-await`
* `--experimental-report`
* `--experimental-resolve-self`
* `--experimental-specifier-resolution`
* `--experimental-vm-modules`
* `--experimental-wasm-modules`
* `--force-context-aware`
Expand Down
4 changes: 2 additions & 2 deletions doc/api/esm.md
Expand Up @@ -1369,7 +1369,7 @@ the CommonJS loader. One of the behavior differences is automatic resolution
of file extensions and the ability to import directories that have an index
file.

The `--es-module-specifier-resolution=[mode]` flag can be used to customize
The `--experimental-specifier-resolution=[mode]` flag can be used to customize
the extension resolution algorithm. The default mode is `explicit`, which
requires the full path to a module be provided to the loader. To enable the
automatic extension resolution and importing from directories that include an
Expand All @@ -1380,7 +1380,7 @@ $ node index.mjs
success!
$ node index # Failure!
Error: Cannot find module
$ node --es-module-specifier-resolution=node index
$ node --experimental-specifier-resolution=node index
success!
```

Expand Down
6 changes: 3 additions & 3 deletions doc/node.1
Expand Up @@ -110,9 +110,6 @@ Enable FIPS-compliant crypto at startup.
Requires Node.js to be built with
.Sy ./configure --openssl-fips .
.
.It Fl -es-module-specifier-resolution
Select extension resolution algorithm for ES Modules; either 'explicit' (default) or 'node'
.
.It Fl -experimental-conditional-exports
Enable experimental support for "require" and "node" conditional export targets.
.
Expand All @@ -130,6 +127,9 @@ Enable experimental top-level
.Sy await
keyword support in REPL.
.
.It Fl -experimental-specifier-resolution
Select extension resolution algorithm for ES Modules; either 'explicit' (default) or 'node'
.
.It Fl -experimental-report
Enable experimental
.Sy diagnostic report
Expand Down
12 changes: 8 additions & 4 deletions lib/internal/modules/esm/default_resolve.js
Expand Up @@ -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,
Expand Down Expand Up @@ -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') {
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, @pd4d10 has been working on C++ warnings for these flags in #30617. The approach there could apply to this PR, depending on which merges first.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 };
}
Expand Down
4 changes: 2 additions & 2 deletions src/module_wrap.cc
Expand Up @@ -789,7 +789,7 @@ inline Maybe<URL> ResolveIndex(const URL& search) {
Maybe<URL> FinalizeResolution(Environment* env,
const URL& resolved,
const URL& base) {
if (env->options()->es_module_specifier_resolution == "node") {
if (env->options()->experimental_specifier_resolution == "node") {
Maybe<URL> file = ResolveExtensions<TRY_EXACT_NAME>(resolved);
if (!file.IsNothing()) {
return file;
Expand Down Expand Up @@ -1053,7 +1053,7 @@ Maybe<URL> PackageMainResolve(Environment* env,
return Just(resolved);
}
}
if (env->options()->es_module_specifier_resolution == "node") {
if (env->options()->experimental_specifier_resolution == "node") {
if (pcfg.has_main == HasMain::Yes) {
return FinalizeResolution(env, URL(pcfg.main, pjson_url), base);
} else {
Expand Down
26 changes: 22 additions & 4 deletions src/node_options.cc
Expand Up @@ -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");
}
}

Expand Down Expand Up @@ -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",
"",
Copy link
Member Author

Choose a reason for hiding this comment

The 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 node -h MAGIC!

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably adopt the same for the aliasing of --experimental-loader and --loader actually.

Copy link
Member

Choose a reason for hiding this comment

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

If this is an alias, shouldn't it be using AddAlias() rather than AddOption()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

¯\_(ツ)_/¯ cc @addaleax

Copy link
Member

Choose a reason for hiding this comment

The 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

I guess we could add a special kind of alias for this, but … why?

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, I think this could use at least Implies() to clarify the relation between the two options

Copy link
Member Author

Choose a reason for hiding this comment

The 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",
Expand Down
1 change: 1 addition & 0 deletions src/node_options.h
Expand Up @@ -104,6 +104,7 @@ class EnvironmentOptions : public Options {
bool experimental_conditional_exports = false;
bool experimental_json_modules = false;
bool experimental_resolve_self = false;
std::string experimental_specifier_resolution;
std::string es_module_specifier_resolution;
bool experimental_wasm_modules = false;
std::string module_type;
Expand Down
16 changes: 16 additions & 0 deletions test/es-module/test-esm-specifiers-both-flags.mjs
@@ -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
}));
18 changes: 18 additions & 0 deletions test/es-module/test-esm-specifiers-legacy-flag.mjs
@@ -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');
2 changes: 1 addition & 1 deletion test/es-module/test-esm-specifiers.mjs
@@ -1,4 +1,4 @@
// Flags: --es-module-specifier-resolution=node
// Flags: --experimental-specifier-resolution=node
import { mustNotCall } from '../common/index.mjs';
import assert from 'assert';

Expand Down
Expand Up @@ -85,6 +85,7 @@ const undocumented = difference(process.allowedNodeEnvironmentFlags,
documented);
// Remove intentionally undocumented options.
assert(undocumented.delete('--debug-arraybuffer-allocations'));
assert(undocumented.delete('--es-module-specifier-resolution'));
assert(undocumented.delete('--experimental-worker'));
assert(undocumented.delete('--no-node-snapshot'));
assert(undocumented.delete('--loader'));
Expand Down