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(perf): lazy load un-common dependencies for npm run #87

Merged
merged 1 commit into from
Apr 7, 2024

Conversation

H4ad
Copy link
Contributor

@H4ad H4ad commented Apr 6, 2024

I know we avoid lazy loading but I think this case is worthy:

Before:

$ hyperfine --warmup 3 "node ./bin/npm-cli.js run empty"
Benchmark 1: node ./bin/npm-cli.js run empty
  Time (mean ± σ):     197.3 ms ±   3.5 ms    [User: 183.1 ms, System: 51.9 ms]
  Range (min … max):   193.2 ms … 207.5 ms    15 runs

After:

$ hyperfine --warmup 3 "node ./bin/npm-cli.js run empty"
Benchmark 1: node ./bin/npm-cli.js run empty
  Time (mean ± σ):     184.6 ms ±   2.3 ms    [User: 170.1 ms, System: 51.1 ms]
  Range (min … max):   181.6 ms … 190.5 ms    16 runs

@H4ad H4ad requested a review from a team as a code owner April 6, 2024 02:54
@lukekarrys lukekarrys changed the title perf: lazy load un-common dependencies for npm run fix(perf): lazy load un-common dependencies for npm run Apr 6, 2024
@wraithgar
Copy link
Member

We need to be 100% sure this code path isn't hit when running npm i -g npm

@wraithgar
Copy link
Member

normalizeData and gitHead are run as part of the prepare process. This is used by npm when preparing a package.json for publishing. That is not done during an install so I think this is safe for npm i -g npm

@wraithgar
Copy link
Member

Additionally, thinking of potential future problems. We are moving in the direction of doing fewer normalizations, not more. So the likelihood of us adding either of those steps during install is very low.

@wraithgar
Copy link
Member

Eventually normalize-package-data needs to be brought under the umbrella of this package. Eventually.

@wraithgar wraithgar merged commit fda5722 into npm:main Apr 7, 2024
20 of 21 checks passed
@github-actions github-actions bot mentioned this pull request Apr 6, 2024
@H4ad H4ad deleted the perf/lazy-load-require branch April 7, 2024 16:06
wraithgar pushed a commit that referenced this pull request Apr 9, 2024
🤖 I have created a release *beep* *boop*
---


## [5.0.1](v5.0.0...v5.0.1)
(2024-04-07)

### Bug Fixes

*
[`fda5722`](fda5722)
[#87](#87) perf: lazy load
un-common dependencies for npm run (#87) (@H4ad)
*
[`71f09d6`](71f09d6)
[#88](#88) perf: only import
necessary functions from semver (#88) (@H4ad)

### Documentation

*
[`6ebb3c9`](6ebb3c9)
[#85](#85) readme: fix broken
badge URL (#85) (@10xLaCroixDrinker)

### Chores

*
[`66e0c23`](66e0c23)
[#80](#80) postinstall for
dependabot template-oss PR (@lukekarrys)
*
[`00e4bbb`](00e4bbb)
[#80](#80) bump
@npmcli/template-oss from 4.21.1 to 4.21.3 (@dependabot[bot])
*
[`d784aa8`](d784aa8)
[#77](#77) postinstall for
dependabot template-oss PR (@lukekarrys)
*
[`efeee22`](efeee22)
[#77](#77) bump
@npmcli/template-oss from 4.19.0 to 4.21.1 (@dependabot[bot])
*
[`a4df4cf`](a4df4cf)
[#56](#56) bump
read-package-json from 6.0.4 to 7.0.0 (@dependabot[bot])
*
[`f7c048a`](f7c048a)
[#58](#58) postinstall for
dependabot template-oss PR (@lukekarrys)
*
[`6240313`](6240313)
[#58](#58) bump
@npmcli/template-oss from 4.18.1 to 4.19.0 (@dependabot[bot])
*
[`5ab117c`](5ab117c)
[#57](#57) postinstall for
dependabot template-oss PR (@lukekarrys)
*
[`f56390e`](f56390e)
[#57](#57) bump
@npmcli/template-oss from 4.18.0 to 4.18.1 (@dependabot[bot])

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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