Skip to content

module: fix specifier resolution option value#35098

Merged
Trott merged 1 commit intonodejs:masterfrom
himself65:20200908-fix
Sep 10, 2020
Merged

module: fix specifier resolution option value#35098
Trott merged 1 commit intonodejs:masterfrom
himself65:20200908-fix

Conversation

@himself65
Copy link
Member

@himself65 himself65 commented Sep 8, 2020 •
edited
Loading

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

nodejs-github-bot commented Sep 8, 2020

Review requested:

  • @nodejs/modules

@MylesBorins
Copy link
Contributor

MylesBorins commented Sep 8, 2020

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

himself65 commented Sep 8, 2020

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,

node/src/node_options.cc

Line 101 in ccdd1bd

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

MylesBorins commented Sep 8, 2020

@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

MylesBorins commented Sep 8, 2020

@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

MylesBorins commented Sep 8, 2020

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

addaleax added a commit to addaleax/node that referenced this pull request Sep 8, 2020
@addaleax
doc,test: specify and test CLI option precedence rules
559197b 
Refs: nodejs#35098 (comment)
@addaleax addaleax mentioned this pull request Sep 8, 2020
Closed
4 tasks
@himself65 himself65 requested a review from addaleax September 9, 2020 22:07
addaleax
addaleax approved these changes Sep 9, 2020
View reviewed changes
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 :)

MylesBorins
MylesBorins approved these changes Sep 10, 2020
View reviewed changes
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

nodejs-github-bot commented Sep 10, 2020

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

@Trott
Copy link
Member

Trott commented Sep 10, 2020 •
edited
Loading

Landed in 20a074b0cd55

Landed in 22c52aa

@Trott Trott merged commit 22c52aa into nodejs:master Sep 10, 2020
@himself65 @Trott
module: fix specifier resolution option value
22c52aa 
Fixes: nodejs#35095

PR-URL: nodejs#35098
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax added a commit that referenced this pull request Sep 11, 2020
@addaleax
doc,test: specify and test CLI option precedence rules
02e9255 
Refs: #35098 (comment)

PR-URL: #35106
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2020
@himself65 @ruyadorno
module: fix specifier resolution option value
314483c 
Fixes: #35095

PR-URL: #35098
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2020
@addaleax @ruyadorno
doc,test: specify and test CLI option precedence rules
b75822d 
Refs: #35098 (comment)

PR-URL: #35106
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@ruyadorno ruyadorno mentioned this pull request Sep 21, 2020
Merged
4 tasks
addaleax added a commit that referenced this pull request Sep 22, 2020
@addaleax
doc,test: specify and test CLI option precedence rules
b47172d 
Refs: #35098 (comment)

PR-URL: #35106
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
@himself65 @MylesBorins
module: fix specifier resolution option value
475af61 
Fixes: #35095

PR-URL: #35098
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
Merged
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@himself65 @MylesBorins
module: fix specifier resolution option value
992af4e 
Fixes: #35095

PR-URL: #35098
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@himself65 @joesepi
module: fix specifier resolution option value
a3449bf 
Fixes: nodejs#35095

PR-URL: nodejs#35098
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
@addaleax @joesepi
doc,test: specify and test CLI option precedence rules
c658026 
Refs: nodejs#35098 (comment)

PR-URL: nodejs#35106
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@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

Reviewers

@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

+1 more reviewer

@MylesBorins MylesBorins MylesBorins approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ESM specifier resolution flag does not use useESMLoader for entry point

7 participants

@himself65 @nodejs-github-bot @MylesBorins @addaleax @bmeck @Trott @GeoffreyBooth