module: fix specifier resolution option value by himself65 · Pull Request #35098 · nodejs/node · GitHub

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

Review requested:

@nodejs/modules

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

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

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

@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

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

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

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

@addaleax SGTM.

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

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.

@addaleax any behavior seems fine, just want docs.

Refs: nodejs#35098 (comment)

Thank you :)

LGTM

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

Landed in 20a074b0cd55

Landed in 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>

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>

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>

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>

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>

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>

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>

addaleax added a commit to addaleax/node that referenced this pull request Sep 8, 2020

doc,test: specify and test CLI option precedence rules

559197b

Refs: nodejs#35098 (comment)

addaleax mentioned this pull request Sep 8, 2020

doc,test: specify and test CLI option precedence rules #35106

Closed

4 tasks

GeoffreyBooth approved these changes Sep 8, 2020

View reviewed changes

himself65 requested a review from addaleax September 9, 2020 22:07

addaleax approved these changes Sep 9, 2020

View reviewed changes

MylesBorins approved these changes Sep 10, 2020

View reviewed changes

Trott approved these changes Sep 10, 2020

View reviewed changes

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 10, 2020

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 10, 2020

Trott force-pushed the 20200908-fix branch from 6788d27 to 20a074b Compare September 10, 2020 14:56

Trott merged commit 22c52aa into nodejs:master Sep 10, 2020

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>

ruyadorno mentioned this pull request Sep 21, 2020

V14.12.0 proposal #35285

Merged

4 tasks

MylesBorins mentioned this pull request Nov 3, 2020

v12.20.0 proposal #35950

Merged

himself65 deleted the 20200908-fix branch April 6, 2023 19:24

module: fix specifier resolution option value by himself65 · Pull Request #35098 · nodejs/node · GitHub

Uh oh!

Uh oh!

module: fix specifier resolution option value - #35098

module: fix specifier resolution option value#35098Trott merged 1 commit intonodejs:masternodejs/node:masterfrom himself65:20200908-fixhimself65/node:20200908-fixCopy head branch name to clipboard

Conversation

himself65 commented Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Sep 8, 2020

Uh oh!

MylesBorins commented Sep 8, 2020

Uh oh!

himself65 commented Sep 8, 2020

Uh oh!

addaleax commented Sep 8, 2020

Uh oh!

MylesBorins commented Sep 8, 2020

Uh oh!

addaleax commented Sep 8, 2020

Uh oh!

MylesBorins commented Sep 8, 2020

Uh oh!

addaleax commented Sep 8, 2020

Uh oh!

MylesBorins commented Sep 8, 2020

Uh oh!

bmeck commented Sep 8, 2020

Uh oh!

addaleax commented Sep 8, 2020

Uh oh!

bmeck commented Sep 8, 2020

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Sep 10, 2020

Uh oh!

Trott commented Sep 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

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

himself65 commented Sep 8, 2020 •
edited
Loading

Trott commented Sep 10, 2020 •
edited
Loading