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: lazy load validate npm package name on error message #7347

Merged
merged 1 commit into from
Apr 9, 2024

Conversation

H4ad
Copy link
Contributor

@H4ad H4ad commented Apr 6, 2024

image

This package takes 5ms to load even if we didn't use it.

@H4ad H4ad requested a review from a team as a code owner April 6, 2024 03:49
@H4ad H4ad force-pushed the perf/startup-time-error-message branch from db5843f to 91af546 Compare April 6, 2024 16:58
@H4ad H4ad changed the title perf: lazy load validate npm package name on error message chore: lazy load validate npm package name on error message Apr 6, 2024
@wraithgar
Copy link
Member

wraithgar commented Apr 6, 2024

TLDR: normally lazy loading in this module is a 🚨 warning zone 🚨 but not for this particular code path.

The danger in lazy loading (especially in this file) is when npm is installing over itself. Unless I'm missing something really obscure I can't imagine why this code path would be called during that situation.

Even if we were doing an npm i -g npm, this error would only happen if we got a 404 from the registry. That would only happen on npm itself (because npm bundles all of its dependencies) meaning that we'd be aborting long before we removed the existing global npm package.

$ npm i -g npm@latest --loglevel verbose
npm verb cli /Users/wraithgar/.nvm/versions/node/v20.10.0/bin/node /Users/wraithgar/.nvm/versions/node/v20.10.0/bin/npm
npm info using [email protected]
npm info using [email protected]
npm verb title npm i npm@latest
npm verb argv "i" "--global" "npm@latest" "--loglevel" "verbose"
npm verb logfile logs-max:10 dir:/Users/wraithgar/.npm/_logs/2024-04-06T18_08_01_736Z-
npm verb logfile /Users/wraithgar/.npm/_logs/2024-04-06T18_08_01_736Z-debug-0.log
npm http fetch GET 200 https://registry.npmjs.org/npm 234ms (cache revalidated)
npm http fetch GET 200 https://registry.npmjs.org/npm 11ms (cache hit)

changed 14 packages in 12s
npm verb exit 0
npm info ok 

@wraithgar wraithgar changed the title chore: lazy load validate npm package name on error message fix: lazy load validate npm package name on error message Apr 6, 2024
@wraithgar
Copy link
Member

@H4ad chore is only for things that won't affect the published module. Refresher on our conventional commit approach here: https://github.com/npm/cli/blob/latest/CONTRIBUTING.md#pull-request-conventions

@wraithgar
Copy link
Member

Approved but I'm gonna ask @lukekarrys to either merge or 👍 for a sensibility check since this is dealing w/ lazy loading and we should be extra cautious.

@wraithgar wraithgar merged commit 5fc0f9d into npm:latest Apr 9, 2024
22 checks passed
@github-actions github-actions bot mentioned this pull request Apr 9, 2024
@H4ad H4ad deleted the perf/startup-time-error-message branch April 9, 2024 20:49
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.

2 participants