-
Notifications
You must be signed in to change notification settings - Fork 3.9k
fix: correctly parse browser as config as string #6020
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
Conversation
|
no statistically significant performance changes detected timing results
|
lib/utils/config/definitions.js
Outdated
| OS X: \`"open"\`, Windows: \`"start"\`, Others: \`"xdg-open"\` | ||
| `, | ||
| type: [null, Boolean, String], | ||
| type: BooleanOrString, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can still be null right? That's how the default works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meant to mark this as draft as i was still experimenting with some things, but what I've found so far:
default: nulldoesn't require thattypecontainsnull- if
typecontainsnulland notBooleanthen anyfalsevalue is coerced back to null. so--no-browserwould end upnull, which wouldn't break anything currently but i think is a major footgun
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if a user wants to provide a value in a project npmrc to override a user npmrc, with the default? iow, an explicit null
|
@wraithgar @lukekarrys Would it be reasonable to break out the This PR overall seems to be attempting to be attempting something much broader to improve testing and linting. |
|
Related #6313 |
WHAT
🤖 Generated by Copilot at 5ef6c04
This pull request introduces a new
mock-globalspackage to the npm cli project, which provides a way to mock the global objects and variables used by the npm cli code. It also updates the code, tests, documentation, and configuration files of the npm cli project to use themock-globalspackage and to improve the quality and compatibility of the code. Additionally, it adds a new GitHub Actions workflow to run linting and testing for themock-globalspackage, and a new.eslintrc.local.jsfile to enforce es6 syntax and restrict require statements for certain files.
🤖 Generated by Copilot at 5ef6c04
WHY
HOW
🤖 Generated by Copilot at 5ef6c04
index.js(link, link)mock-globalspackage to provide a mock implementation of global variables for testing (link, link, link, link, link, link, link, link, link, link, link, link, link, link)npmcli-configpackage to handle npm config options and constants (link, link, link, link, link, link, link, link, link, link)tmockmodule to mockNpmclass and its dependencies inmock-npmmodule (link, link, link, link, link)noptpackage instead of downloading it from npm registry (link)npmcli-docspackage as a dependency ofconfigpackage to generate documentation for config options (link, link)configpackage (link, link, link, link)npmpackage (link, link)docspackage to reflect changes inconfigpackage (link)