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

Conversation

MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented Nov 27, 2019

--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.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 27, 2019
@MylesBorins
Copy link
Contributor Author

/cc @nodejs/modules

@MylesBorins MylesBorins added the esm Issues and PRs related to the ECMAScript Modules implementation. label Nov 27, 2019
@MylesBorins
Copy link
Contributor Author

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 MylesBorins force-pushed the experimental-specifier-resolution branch from 13f726a to db9f3ec Compare November 27, 2019 09:06
@MylesBorins
Copy link
Contributor Author

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

-->

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.

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Nov 27, 2019 via email

@MylesBorins
Copy link
Contributor Author

MylesBorins commented Nov 28, 2019 via email

@guybedford
Copy link
Contributor

implicit/explicit sounds great to me.

@GeoffreyBooth
Copy link
Member

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 MylesBorins force-pushed the experimental-specifier-resolution branch from e6af0a6 to 118171c Compare November 28, 2019 08:17
@MylesBorins
Copy link
Contributor Author

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

-->

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

This comment was marked as resolved.

@@ -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
Contributor 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
Contributor 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?

src/node_options.cc Show resolved Hide resolved
&EnvironmentOptions::experimental_specifier_resolution,
kAllowedInEnvironment);
AddOption("--es-module-specifier-resolution",
"",
Copy link
Contributor 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
Contributor 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
Contributor 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.

@guybedford 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
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joaocgreis
Copy link
Member

@MylesBorins
Copy link
Contributor Author

Landed in ee953d8

@MylesBorins MylesBorins closed this Dec 4, 2019
MylesBorins added a commit that referenced this pull request Dec 4, 2019
`--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 <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
@guybedford guybedford mentioned this pull request Dec 5, 2019
2 tasks
targos pushed a commit that referenced this pull request Dec 9, 2019
`--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 <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 13, 2019
MylesBorins added a commit that referenced this pull request Jan 12, 2020
`--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 <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
`--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 <[email protected]>
Reviewed-By: Guy Bedford <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants