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.

Sign up for GitHub

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

Jump to bottom

Fix typo in --specifier-resolution=[node] #39249

Closed
wants to merge 3 commits into from
Closed

Fix typo in --specifier-resolution=[node] #39249

wants to merge 3 commits into from

Conversation

Urigo
Copy link

@Urigo Urigo commented Jul 3, 2021

No description provided.

@github-actions github-actions bot added doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. labels Jul 3, 2021
mscdex
mscdex reviewed Jul 3, 2021
View reviewed changes
doc/api/esm.md Outdated
@@ -1303,7 +1303,7 @@ the CommonJS loader. One of the behavior differences is automatic resolution
of file extensions and the ability to import directories that have an index
file.

The `--experimental-specifier-resolution=[mode]` flag can be used to customize
The `--experimental-specifier-resolution=[node]` flag can be used to customize
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this change is correct. "mode" here is referring to the parameter name, whereas "node" is one possible value for the "mode" parameter. The "mode" could also have a value of "explicit".

benjamingr
benjamingr requested changes Jul 3, 2021
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment •
edited

Choose a reason for hiding this comment

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

Thanks for this PR like mscdex said this is not a typo :)

Changes to the docs making that clearer are welcome though I'm personally not sure how to make it clearer.

@Urigo
Copy link
Author

Urigo commented Jul 3, 2021

ohh got it, sorry about that :)

mmm I guess maybe something different then mode? maybe resolution-mode ?

@gireeshpunathil
Copy link
Member

mmm I guess maybe something different then mode? maybe resolution-mode ?

from the very fact that this PR exists shows that the word mode is confusing, and makes sense to change it, though resolution-mode looks a bit longer for the purpose?

/cc @nodejs/loaders

@benjamingr
Copy link
Member

I think resolution-mode is perfectly fine (especially since it's only for the docs example and not the actual flag) :)

PR update welcome

@Urigo
Copy link
Author

Urigo commented Jul 4, 2021

Thanks for the comments!
Updated

tniessen
tniessen reviewed Jul 5, 2021
View reviewed changes
doc/api/esm.md Outdated
@@ -1303,7 +1303,7 @@ the CommonJS loader. One of the behavior differences is automatic resolution
of file extensions and the ability to import directories that have an index
file.

The `--experimental-specifier-resolution=[mode]` flag can be used to customize
The `--experimental-specifier-resolution=[resolution-mode]` flag can be used to customize
Copy link
Member

Choose a reason for hiding this comment

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

This line now exceeds 80 characters, while the rest of the paragraph is wrapped at 80 chars. Not sure whether we usually fix this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it fails the lint doc check, which blocks merging.

Copy link
Member

Choose a reason for hiding this comment

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

@tniessen in my anecdotal experience we usually fix this for first time contributors if they are unable to do so themselves.

benjamingr
benjamingr approved these changes Jul 6, 2021
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Fix looks good - please drop it to two lines in order to meet the 80 character limit :)

@aduh95
Copy link
Contributor

aduh95 commented Apr 22, 2023

The flag was removed in #44859.

@aduh95 aduh95 closed this Apr 22, 2023
@benjamingr
Copy link
Member

benjamingr commented Apr 24, 2023 •
edited

Sorry for dropping the ball on getting this through the finish line back then @Urigo ! Thank you for your contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@benjamingr benjamingr benjamingr approved these changes

@JakobJingleheimer JakobJingleheimer JakobJingleheimer left review comments

@tniessen tniessen tniessen left review comments

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@aduh95 aduh95 aduh95 approved these changes

@HarshithaKP HarshithaKP HarshithaKP approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Urigo @gireeshpunathil @benjamingr @aduh95 @mscdex @JakobJingleheimer @tniessen @HarshithaKP