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

shrinkwrap: no need to read package.json when read shrinkwrap #504

Closed
wants to merge 3 commits into from

Conversation

lou1swu
Copy link
Contributor

@lou1swu lou1swu commented Nov 20, 2019

The PR includes a simple change that removes the unnecessary operation when readShrinkwrap.
This change will optimize performance when npm install.

@lou1swu lou1swu requested a review from a team as a code owner November 20, 2019 12:20
Copy link
Contributor

@isaacs isaacs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor changes. Otherwise, yes, I can see that value isn't being used in this function, so it should be safe to remove the unnecessary file read.

@@ -31,7 +31,7 @@ function readShrinkwrap (child, next) {
}
child.package._shrinkwrap = parsed
}
).then(() => next(), next)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a functional change outside the scope of the intent of this PR. It means that a return value will be passed to the next function as the first argument, which would be interpreted as an error. Currently, no return value is being provided, but it makes the code more brittle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I would undo this change.

lib/install/read-shrinkwrap.js Show resolved Hide resolved
no need to read package.json when read shrinkwrap.
avoid to read package.json when read shrinkwrap.
@lou1swu lou1swu requested a review from isaacs November 21, 2019 08:17
@isaacs
Copy link
Contributor

isaacs commented Nov 21, 2019

This looks good to me now. We'll review it for the next 6.x release. Thanks!

@lou1swu lou1swu closed this Nov 22, 2019
@lou1swu lou1swu reopened this Nov 22, 2019
@mikemimik mikemimik added semver:patch semver patch level for changes Release 6.x work is associated with a specific npm 6 release labels Nov 26, 2019
@mikemimik mikemimik added this to the Release 6.14.0 milestone Nov 26, 2019
@mikemimik mikemimik closed this in e4b9796 Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release 6.x work is associated with a specific npm 6 release semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants