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.

Sign up for GitHub

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

Jump to bottom

perf: only import necessary functions from semver #88

Merged
merged 1 commit into from
Apr 6, 2024
Merged

perf: only import necessary functions from semver #88

merged 1 commit into from
Apr 6, 2024

Conversation

H4ad
Copy link
Contributor

@H4ad H4ad commented Apr 6, 2024

Just saving a couple of ms by only importing what is needed.

@H4ad H4ad requested a review from a team as a code owner April 6, 2024 16:28
@lukekarrys lukekarrys merged commit 71f09d6 into npm:main Apr 6, 2024
@github-actions github-actions bot mentioned this pull request Apr 6, 2024
Merged
@H4ad H4ad deleted the perf/semver branch April 6, 2024 16:48
@wraithgar
Copy link
Member

Going forward I would like to think about these things a little more holistically. require is a singleton, and we require('semver') EVERYWHERE in npm. This is now a distinct require that can not share that singleton state with the rest of npm.

@H4ad
Copy link
Contributor Author

H4ad commented Apr 6, 2024

Your thinking is right but the conclusion is wrong, we still share the state but now we distribute the loading time across the entire module execution instead of having a huge load in the beginning.

Also, if we introduce new features to semver, this will affect negatively this module since we are importing the entire package, if we import only what is needed, this is very unlikely to happen.

And if we load semver package entirely and then semver/functions/satisfies, the second call will get the module from the cache.

@wraithgar
Copy link
Member

Awesome! Thanks for the clarification!

wraithgar pushed a commit that referenced this pull request Apr 9, 2024
@github-actions
chore: release 5.0.1 (#79)
d4814d4 
🤖 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
Reviewers

@lukekarrys lukekarrys lukekarrys approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@H4ad @wraithgar @lukekarrys