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

install/dedupe: fix hoisting of packages with peerDeps #147

Merged
merged 8 commits into from
Jan 29, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions lib/dedupe.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,12 @@ function hoistChildren_ (tree, diff, seen, next) {
[andComputeMetadata(tree)]
], done)
}
var hoistTo = earliestInstallable(tree, tree.parent, child.package, log)
if (hoistTo) {
// find a location where it's installable
// child is marked as removed so it's ignored when finding a location
child.removed = true
var hoistTo = earliestInstallable(tree, tree, child.package, log)
child.removed = false
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'd rather add an argument here than patch a child statefully like this. This otherwise makes sense.

if (hoistTo && hoistTo !== tree) {
move(child, hoistTo, diff)
chain([
[andComputeMetadata(hoistTo)],
Expand Down
15 changes: 15 additions & 0 deletions lib/install/deps.js
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,21 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr

if (tree.phantomChildren && tree.phantomChildren[pkg.name]) return null

// if any of the peer dependencies is a (dev) dependency of the current
// tree locations, we place the package here. This is a safe location
// where we don't violate the peer dependencies contract.
// It may not be the perfect location in same cases, but we don't know
sokra marked this conversation as resolved.
Show resolved Hide resolved
// if (dev) dependencies are hoisted to another location yet, as they
// may not be loaded into the tree yet.
var peerDeps = pkg.peerDependencies
if (peerDeps) {
if (Object.keys(peerDeps).some(function (name) {
return deps[name] || devDeps[name]
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have a check where we only check devDeps if tree.isTop (see the var devDeps ... section just above this one). We think this is otherwise fine and actually a pretty nice patch!

})) {
return tree
}
}

if (tree.isTop) return tree
if (tree.isGlobal) return tree

Expand Down