-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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): do less work loading config #7361
Conversation
npm tests fixed, added a config test to make sure we keep the definitions sorted (will help guard against the one thing |
@@ -695,6 +687,7 @@ class Config { | |||
} | |||
|
|||
if (this.localPrefix && hasPackageJson) { | |||
const rpj = require('read-package-json-fast') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use none of the normalization here, we should consider using the underlying JSON parser in this situation.
|
||
const semver = require('semver') | ||
class Umask {} | ||
class Semver {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@H4ad the class is meaningless it's just a unique "object" to hang nopt definitions off of. I opted for the same as Umask
here instead. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it doesn't matter, I think it's okay.
I'd rather have our own version than import from semver
, even though the import time from classes/semver
is being measured in us
.
This adds some lazy loading, inlines some single use functions, and also
removes the "define" function in the definitions file. That was
guarding against something that isn't worth the runtime to check for.