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

module: fix specifier resolution option value #35098

Merged
merged 1 commit into from
Sep 10, 2020

Conversation

himself65
Copy link
Member

@himself65 himself65 commented Sep 8, 2020

Fixes: #35095

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/modules

@MylesBorins
Copy link
Contributor

The two flags are already set up as aliases in the options parser FWIW

https://github.com/nodejs/node/blob/master/src/node_options.cc#L95-L114

https://github.com/nodejs/node/blob/master/src/node_options.cc#L366-L374

@himself65
Copy link
Member Author

The two flags are already set up as aliases in the options parser FWIW

https://github.com/nodejs/node/blob/master/src/node_options.cc#L95-L114

https://github.com/nodejs/node/blob/master/src/node_options.cc#L366-L374

Yep, so I found what the actually bug is,

experimental_specifier_resolution = es_module_specifier_resolution;

assign experimental_specifier_resolution with es_module_specifier_resolution but what if we have experimental_specifier_resolution=node instead of es_module_specifier_resolution

@addaleax
Copy link
Member

addaleax commented Sep 8, 2020

Why is --es-module-specifier-resolution not set up as an actual alias of --experimental-specifier-resolution (or vice versa) using AddAlias()? That doesn’t seem great in the first place

@MylesBorins
Copy link
Contributor

@addaleax I think I set it up this way to ensure that both aliases were not used at the same time, which may have been a bit of overengineering. We could likely simplify the logic or simply remove the alias on master / 15 tbh

@addaleax
Copy link
Member

addaleax commented Sep 8, 2020

@MylesBorins Yeah, I think that’s something we should definitely simplify. Having it be a plain alias turns it into a single line with basically 0 maintenance overhead (which can and should be kept indefinitely according to our policies).

@himself65 Do you want to turn --experimental-specifier-resolution into an alias for --es-module-specifier-resolution yourself, or should I open a PR with that?

@MylesBorins
Copy link
Contributor

@addaleax would a simple alias fail if both the original and alias are passed with different values?

@addaleax
Copy link
Member

addaleax commented Sep 8, 2020

@MylesBorins No, but if necessary we could make that happen. However, I don’t think it’s worth all the complexity of this code.

@MylesBorins
Copy link
Contributor

@addaleax SGTM.

@bmeck
Copy link
Member

bmeck commented Sep 8, 2020

Do we have a documented priority of the option value if 2 conflicting values are supplied?

@addaleax
Copy link
Member

addaleax commented Sep 8, 2020

Do we have a documented priority of the option value if 2 conflicting values are supplied?

The last specified value is used. This is just like when an option that has a single value is used twice.

@bmeck
Copy link
Member

bmeck commented Sep 8, 2020

@addaleax any behavior seems fine, just want docs.

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Thank you :)

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 10, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 10, 2020
@nodejs-github-bot
Copy link
Collaborator

@Trott
Copy link
Member

Trott commented Sep 10, 2020

Landed in 20a074b0cd55

Landed in 22c52aa

@Trott Trott merged commit 22c52aa into nodejs:master Sep 10, 2020
Fixes: nodejs#35095

PR-URL: nodejs#35098
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
addaleax added a commit that referenced this pull request Sep 11, 2020
Refs: #35098 (comment)

PR-URL: #35106
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2020
Fixes: #35095

PR-URL: #35098
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2020
Refs: #35098 (comment)

PR-URL: #35106
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Sep 21, 2020
4 tasks
addaleax added a commit that referenced this pull request Sep 22, 2020
Refs: #35098 (comment)

PR-URL: #35106
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
Fixes: #35095

PR-URL: #35098
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
Fixes: #35095

PR-URL: #35098
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Fixes: nodejs#35095

PR-URL: nodejs#35098
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Refs: nodejs#35098 (comment)

PR-URL: nodejs#35106
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@himself65 himself65 deleted the 20200908-fix branch April 6, 2023 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESM specifier resolution flag does not use useESMLoader for entry point
7 participants