perf: avoid importing npm-registry-fetch for replace-info #7314
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
We are going to publish the replace info util as its own module since we need a version of it elsewhere as well. I believe that should have a similar performance impact. But I don't think this PR will land as is. Thank you for this though! I will keep the open until we have an open PR implementing it as a separate package so we can benchmark that also. |
|
In case you are interested in other refactor or small gains, the full log with the amount of time to load each js file requested: npm run echo |
|
See prior discussion in #7176 |
|
Just a refresher here about why npm does not typically allow lazy loading of modules. |
|
I also want to say again thank you @H4ad for all of the work you've put in on performance improvements to npm. It's nowhere near as easy as folks think it is, it's slow going and usually when you're done nobody even notices. We notice. We think you're great. |
|
imho replace-info and cleanUrl belong in this net new package, wdyt @lukekarrys ? (This is now the issue where we will discuss the new package, it's as good as any) |
I also want to add that we now have a test protecting against regressions here. We (and by we I mean @wraithgar) have had success moving to lazy loading requires in a few places after we implemented that test. |
agreed. what do we think about a name? |
|
This does more than clean urls it also has a more aggressive match for stripping known npm token patterns. |
|
Or we can make this a named export of our logger which we will be requiring on every instantiation. |
|
Maybe this will springboard into |
|
output / logging task is here we can move the conversation there |
lukekarrys
added a commit
that referenced
this pull request
Apr 3, 2024
|
Closing in favor of #7334 |
lukekarrys
added a commit
that referenced
this pull request
Apr 3, 2024
lukekarrys
added a commit
that referenced
this pull request
Apr 3, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of importing the entire module, just copy the function from the
npm-registry-fetch.Benchmark:
hyperfine --warmup 3 "node ./bin/npm-cli.js run echo"Before:
After: