esm: make specifier flag clearly experimental #30678

MylesBorins · 2019-11-27T08:12:23Z

--es-module-specifier-resolution is the only flagged portion of the
ESM implementation that does not have the word experimental in the flag
name. This commit changes the flag to:

--experimental-specifier-resolution

--es-module-specifier-resolution remains as an alias for backwards
compatibility but it is no longer documented.

There is a failing test with this PR test/es-module/test-esm-specifiers-both-flags.mjs. There is not yet handling to ensure both flags can't be used at the same time... unsure the best way to do this with the alias.

~~It does fail with the wrong message though, and doesn't fail when arguments are in the opposite direction. This might be an unrelated bug in the options parser.~~

`--es-module-specifier-resolution` is the only flagged portion of the ESM implementation that does not have the word experimental in the flag name. This commit changes the flag to: `--experimental-specifier-resolution` `--es-module-specifier-resolution` remains as an alias for backwards compatibility but it is no longer documented.

MylesBorins · 2019-11-27T08:12:33Z

/cc @nodejs/modules

MylesBorins · 2019-11-27T08:35:54Z

I believe that not having this flag prefixed by experimental was an oversight. Prior art in renaming another flag before we unflagged modules

15fb02a

MylesBorins · 2019-11-27T09:07:12Z

It now gives a reasonable warning if you attempt to use both flags at the same time.

guybedford · 2019-11-27T22:29:58Z

doc/api/cli.md

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


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?

Maybe --experimental-specifier-resolution=require to make it obvious what it is emulating?

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.

Would --experimental-specifier-resolution=detect capture the semantics?

I'm happy to go with what we have though, and have approved the PR.

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

errr, if you want a new name, why not implicit to mirror explicit?

MylesBorins · 2019-11-27T23:05:57Z

I don't think we should be changing the values. It might not be ideal, but at least this PR should be non disruptive as it is backwards compatible. If we are going to change the values I think it would make sense to do before officially supporting the feature if we go that route

…

On Wed, Nov 27, 2019, 5:57 PM Guy Bedford ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In doc/api/cli.md <#30678 (comment)>: > @@ -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` + + +Sets the resolution algorithm for resolving ES module specifiers. Valid options +are `explicit` and `node`. I'm happy to go with what we have though, and have approved the PR. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#30678?email_source=notifications&email_token=AADZYV5JYOO6HN3EQYGNGILQV33PJA5CNFSM4JSDAMT2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCNHZH6Q#discussion_r351528532>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADZYV7DUJZJKOUAA37IFZ3QV33PJANCNFSM4JSDAMTQ> .

MylesBorins · 2019-11-28T00:17:29Z

We could document / support a new name but keep the old name "node" supported but undocumented. Thoughts? I like implicit

…

On Wed, Nov 27, 2019, 7:07 PM Wesley Wigham ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In doc/api/cli.md <#30678 (comment)>: > @@ -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` + + +Sets the resolution algorithm for resolving ES module specifiers. Valid options +are `explicit` and `node`. errr, if you want a new name, why not implicit to mirror explicit? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#30678?email_source=notifications&email_token=AADZYV62DIZJP5ROWMJ74U3QV4DS3A5CNFSM4JSDAMT2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCNH5FNI#discussion_r351542524>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADZYVZT5PUIS7BO7RQG5ITQV4DS3ANCNFSM4JSDAMTQ> .

guybedford · 2019-11-28T00:19:49Z

implicit/explicit sounds great to me.

GeoffreyBooth · 2019-11-28T00:30:17Z

I would just keep node and explicit as they are. implicit to me doesn’t imply that index resolution will happen, and it also (to me) suggests that all extensions will be resolved, rather than just the ones in require.extensions. The node name was well chosen because this is really a Node-specific algorithm, with quirky rules that can’t be summarized in a word or two.

If anything I would just call the flag experimental-es-module-specifier-resolution, so that if people read about references to es-module-specifier-resolution (such as from our own blog posts) it’s clear that this new flag is just a renamed version of that. I agree that if this becomes non-experimental, that’s the time to consider renaming things.

MylesBorins · 2019-11-28T08:18:19Z

I've fixed up broken tests I think this should be good to go

@GeoffreyBooth do you really think we need such a verbose flag? Especially since we are adding "experimental" I think it makes sense tot have the flags be slightly different

doc/api/cli.md

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


MylesBorins · 2019-11-28T08:20:45Z

lib/internal/modules/esm/default_resolve.js

@@ -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') {


should we emit the warning on the use of the flag or only when it is active?

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.

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.

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.

Is getting the warning into C++ layer blocking this from landing or should we do it in a follow up?

src/node_options.cc

MylesBorins · 2019-11-28T08:21:46Z

src/node_options.cc

+            &EnvironmentOptions::experimental_specifier_resolution,
+            kAllowedInEnvironment);
+  AddOption("--es-module-specifier-resolution",
+            "",


Fun fact, leaving no description on a flag stop it from printing when you run node -h MAGIC!

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

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

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?

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

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?

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

If y'all are happy with just shipping this as is, it is a one off and something that will eventually get removed.

test/es-module/test-esm-specifiers-both-flags.mjs

nodejs-github-bot · 2019-12-03T03:29:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27292/

nodejs-github-bot · 2019-12-03T20:30:44Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27312/

test/es-module/test-esm-specifiers-both-flags.mjs

nodejs-github-bot · 2019-12-03T21:42:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/27314/

joaocgreis · 2019-12-03T22:05:27Z

Windows CI rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/31952/

MylesBorins · 2019-12-04T21:22:04Z

Landed in ee953d8

`--es-module-specifier-resolution` is the only flagged portion of the ESM implementation that does not have the word experimental in the flag name. This commit changes the flag to: `--experimental-specifier-resolution` `--es-module-specifier-resolution` remains as an alias for backwards compatibility but it is no longer documented. PR-URL: #30678 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 27, 2019

MylesBorins added the esm Issues and PRs related to the ECMAScript Modules implementation. label Nov 27, 2019

fixup: add experimental warning

60a9dcb

MylesBorins requested review from guybedford, ljharb, jkrems and devsnek November 27, 2019 08:22

MylesBorins mentioned this pull request Nov 27, 2019

doc: clarify extensionless esm #30657

Closed

4 tasks

fixup: support both flags and give proper warnings

db9f3ec

MylesBorins force-pushed the experimental-specifier-resolution branch from 13f726a to db9f3ec Compare November 27, 2019 09:06

jkrems approved these changes Nov 27, 2019

View reviewed changes

guybedford approved these changes Nov 27, 2019

View reviewed changes

MylesBorins added 2 commits November 28, 2019 03:13

fixup: cleanup cli help / man data

f4eb371

fixup: fix broken tests

118171c

MylesBorins force-pushed the experimental-specifier-resolution branch from e6af0a6 to 118171c Compare November 28, 2019 08:17

MylesBorins commented Nov 28, 2019

View reviewed changes

guybedford mentioned this pull request Nov 29, 2019

module: add warnings for experimental flags #30617

Closed

11 tasks

guybedford added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 3, 2019

guybedford requested changes Dec 3, 2019

View reviewed changes

test/es-module/test-esm-specifiers-both-flags.mjs Show resolved Hide resolved

windows possible fix

7bf301e

guybedford approved these changes Dec 3, 2019

View reviewed changes

MylesBorins closed this Dec 4, 2019

guybedford mentioned this pull request Dec 5, 2019

module: fixup lint regression #30802

Closed

2 tasks

MylesBorins mentioned this pull request Dec 13, 2019

v13.4.0 proposal #30937

Merged

targos mentioned this pull request Jan 15, 2020

v12.15.0 release proposal #31368

Closed

MylesBorins mentioned this pull request Feb 8, 2020

v12.16.0 proposal #31691

Merged

esm: make specifier flag clearly experimental #30678

esm: make specifier flag clearly experimental #30678

Conversation

MylesBorins commented Nov 27, 2019 • edited

MylesBorins commented Nov 27, 2019

MylesBorins commented Nov 27, 2019

MylesBorins commented Nov 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb Nov 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

MylesBorins commented Nov 27, 2019 via email

MylesBorins commented Nov 28, 2019 via email

guybedford commented Nov 28, 2019

GeoffreyBooth commented Nov 28, 2019

MylesBorins commented Nov 28, 2019

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 3, 2019

nodejs-github-bot commented Dec 3, 2019

nodejs-github-bot commented Dec 3, 2019

joaocgreis commented Dec 3, 2019

MylesBorins commented Dec 4, 2019

MylesBorins commented Nov 27, 2019 •

edited

ljharb Nov 27, 2019 •

edited