esm: make specifier flag clearly experimental by MylesBorins · Pull Request #30678 · nodejs/node · GitHub

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

/cc @nodejs/modules

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

15fb02a

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

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?

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

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

implicit/explicit sounds great to me.

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.

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

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?

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.

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

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

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

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

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 devsnek, guybedford, hybrist and ljharb 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

hybrist 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 outstanding review comments, and a CI started. label Dec 3, 2019

guybedford requested changes Dec 3, 2019

View reviewed changes

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

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

                   format = getPackageType(url.href) === TYPE_MODULE ? 'module' : 'commonjs';
                 if (!format) {
-                  if (esModuleSpecifierResolution === 'node')
+                  if (experimentalSpeciferResolution === 'node') {

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

esm: make specifier flag clearly experimental by MylesBorins · Pull Request #30678 · nodejs/node · GitHub

Uh oh!

Uh oh!

esm: make specifier flag clearly experimental - #30678

esm: make specifier flag clearly experimental#30678MylesBorins wants to merge 6 commits intonodejs:masternodejs/node:masterfrom MylesBorins:experimental-specifier-resolutionMylesBorins/node:experimental-specifier-resolutionCopy head branch name to clipboard

Conversation

MylesBorins commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented Nov 27, 2019

Uh oh!

MylesBorins commented Nov 27, 2019

Uh oh!

MylesBorins commented Nov 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharb Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

MylesBorins commented Nov 27, 2019 via email

Uh oh!

MylesBorins commented Nov 28, 2019 via email

Uh oh!

guybedford commented Nov 28, 2019

Uh oh!

GeoffreyBooth commented Nov 28, 2019

Uh oh!

MylesBorins commented Nov 28, 2019

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 3, 2019

Uh oh!

nodejs-github-bot commented Dec 3, 2019

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 3, 2019

Uh oh!

esm: make specifier flag clearly experimental#30678
MylesBorins wants to merge 6 commits into
nodejs:masterfrom
MylesBorins:experimental-specifier-resolution

MylesBorins commented Nov 27, 2019 •
edited
Loading

ljharb Nov 27, 2019 •
edited
Loading