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

fix: correctly parse browser as config as string #6020

Closed
wants to merge 9 commits into from

Conversation

lukekarrys
Copy link
Contributor

@lukekarrys lukekarrys commented Jan 3, 2023

WHAT

🤖 Generated by Copilot at 5ef6c04

This pull request introduces a new mock-globals package 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 the mock-globals package and to improve the quality and compatibility of the code. Additionally, it adds a new GitHub Actions workflow to run linting and testing for the mock-globals package, and a new .eslintrc.local.js file to enforce es6 syntax and restrict require statements for certain files.

🤖 Generated by Copilot at 5ef6c04

We're the crew of the npm cli
We work on code and docs with glee
We mock the globals and lint the files
And test them all with tap and tmock

WHY

HOW

🤖 Generated by Copilot at 5ef6c04

  • Move engine validation logic to a separate file and require it from index.js (link, link)
  • Add mock-globals package to provide a mock implementation of global variables for testing (link, link, link, link, link, link, link, link, link, link, link, link, link, link)
  • Use npmcli-config package to handle npm config options and constants (link, link, link, link, link, link, link, link, link, link)
  • Use tmock module to mock Npm class and its dependencies in mock-npm module (link, link, link, link, link)
  • Use local copy of nopt package instead of downloading it from npm registry (link)
  • Add npmcli-docs package as a dependency of config package to generate documentation for config options (link, link)
  • Remove unused or deprecated options and files from config package (link, link, link, link)
  • Update dependency list and hierarchy of npm package (link, link)
  • Update snapshot for docs package to reflect changes in config package (link)

@lukekarrys lukekarrys requested a review from a team as a code owner January 3, 2023 07:41
@npm-cli-bot
Copy link
Collaborator

no statistically significant performance changes detected

timing results
app-large clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 39.826 ±3.55 15.219 ±0.04 14.044 ±0.08 16.665 ±0.95 2.616 ±0.01 2.640 ±0.02 2.112 ±0.00 9.809 ±0.15 2.120 ±0.04 3.865 ±0.97
#6020 32.884 ±6.12 14.792 ±0.02 13.754 ±0.15 16.625 ±1.41 2.625 ±0.03 2.600 ±0.04 2.025 ±0.01 9.708 ±0.04 2.070 ±0.01 3.270 ±0.13
app-medium clean lock-only cache-only cache-only
peer-deps
modules-only no-lock no-cache no-modules no-clean no-clean
audit
npm@8 23.274 ±0.08 11.899 ±0.06 10.973 ±0.05 12.232 ±0.81 2.523 ±0.09 2.408 ±0.03 2.201 ±0.01 7.507 ±0.05 2.071 ±0.01 4.452 ±2.18
#6020 24.330 ±1.33 11.683 ±0.15 10.809 ±0.04 11.875 ±0.13 2.373 ±0.01 2.398 ±0.02 2.097 ±0.02 7.509 ±0.22 1.986 ±0.06 2.821 ±0.03

@@ -278,7 +279,7 @@ define('browser', {
defaultDescription: `
OS X: \`"open"\`, Windows: \`"start"\`, Others: \`"xdg-open"\`
`,
type: [null, Boolean, String],
type: BooleanOrString,
Copy link
Member

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?

Copy link
Contributor Author

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: null doesn't require that type contains null
  • if type contains null and not Boolean then any false value is coerced back to null. so --no-browser would end up null, which wouldn't break anything currently but i think is a major footgun

Copy link
Contributor

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

@lukekarrys lukekarrys marked this pull request as draft January 3, 2023 17:47
@ianlewis
Copy link

@wraithgar @lukekarrys Would it be reasonable to break out the BooleanOrString validation into another PR in order to fix that cli flags can't be a boolean or string? (though I'm guessing that more may be needed to actually solve the problem).

This PR overall seems to be attempting to be attempting something much broader to improve testing and linting.

@ianlewis
Copy link

Related #6313

@wraithgar
Copy link
Member

#6314 (comment)

@wraithgar wraithgar closed this May 18, 2023
@wraithgar wraithgar deleted the lk/browser-string branch May 18, 2023 19:35
@lukekarrys lukekarrys restored the lk/browser-string branch May 18, 2023 20:09
@lukekarrys lukekarrys deleted the lk/browser-string branch May 22, 2023 16:30
@lukekarrys lukekarrys restored the lk/browser-string branch May 22, 2023 16:30
@lukekarrys lukekarrys deleted the lk/browser-string branch May 22, 2023 16:30
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.

5 participants