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

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

Merged
merged 8 commits into from
Jan 29, 2019
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4bd5a3e
install/dedupe: fix hoisting of packages with peerDeps
sokra Jan 24, 2019
214f81e
Fix typo
Kinrany Jan 24, 2019
85b0c5d
ignore devDeps when checking peerDeps
sokra Jan 27, 2019
7dbc8cc
add ignoreChild argument to earliestInstallable
sokra Jan 27, 2019
27b0279
add integration test for peerDependencies
sokra Jan 27, 2019
9b23002
fix maketest script
sokra Jan 27, 2019
0f0f2b4
remove invalid comment
sokra Jan 27, 2019
44953fa
fix linting issues
sokra Jan 28, 2019
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
View file
Open in desktop
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
View file
Open in desktop
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 some cases, but we don't know
// 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