Install shrink-wrapped package dependencies recursively (reference #2931) #2950

Closed
wants to merge 2 commits into
from

Projects

None yet

10 participants

@scf2k
scf2k commented Nov 15, 2012

No description provided.

@arikon
Contributor
arikon commented Nov 15, 2012

@isaacs Have a look at this PR please

@isaacs

Take out of the var statement, it's already defined as a function parameter.

@isaacs

Since this is so much shorter, replace the if/else with an early return. Ie, instead of:

if (cond) {
  // big long bunch of stuff
} else oneLiner();

do this:

if (!cond) return oneLiner();
// big long bunch of stuff

Actually, it'd probably be best to just move this entire block out into its own function. There's a LOT of indentation here, it's kind of crazy.

@isaacs
Member
isaacs commented Jan 15, 2013

Mostly looks pretty good. A few relatively minor nits (commented inline).

@scf2k
scf2k commented Jan 21, 2013

Adjusted the code according to the comments.

@scf2k
scf2k commented Feb 8, 2013

ping

@andyburke

Any word on this? This seems like more intuitive behavior. Whatever versions are specified in the npm-shrinkwrap.json should get installed, even if they're deeper in the dependency tree.

@Raynos
Contributor
Raynos commented May 12, 2014

👍 would love to get this in.

Willing to take over this branch and rebase it & write a test for it.

@Raynos Raynos referenced this pull request in uber/npm-shrinkwrap May 12, 2014
Open

npm install does not verify shrinkwrap correctness #16

@domenic
Member
domenic commented May 14, 2014

@Raynos since the basic idea seems @isaacs-approved, I think that's enough to say "yes, please take this over and write a test for it," and then I or someone else can merge it.

@demisx
demisx commented Aug 21, 2014

Any chance this is gonna make it in some time soon?

@othiym23 othiym23 added this to the multi-stage install milestone Sep 21, 2014
@bestander

ping

@iarna
Member
iarna commented Dec 12, 2014

This isn't going to land as is, as the install code has been completely rewritten for the multi-stage installer and npm@3. But that said, we ARE integrating the feature this implements into the new version.

@iarna
Member
iarna commented Dec 15, 2014

Closing this as this is addressed more generally by #6928

@iarna iarna closed this Dec 15, 2014
@iarna iarna removed the review label Dec 15, 2014
@iarna iarna locked and limited conversation to collaborators Jun 24, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.