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

Revert "install/dedupe: fix hoisting of packages with peerDeps (#147)" #152

Merged
merged 1 commit into from
Feb 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
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
5 changes: 2 additions & 3 deletions lib/dedupe.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,9 +142,8 @@ function hoistChildren_ (tree, diff, seen, next) {
[andComputeMetadata(tree)]
], done)
}
// find a location where it's installable
var hoistTo = earliestInstallable(tree, tree, child.package, log, child)
if (hoistTo && hoistTo !== tree) {
var hoistTo = earliestInstallable(tree, tree.parent, child.package, log)
if (hoistTo) {
move(child, hoistTo, diff)
chain([
[andComputeMetadata(hoistTo)],
Expand Down
29 changes: 6 additions & 23 deletions lib/install/deps.js
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@ function resolveWithNewModule (pkg, tree, log, next) {
return isInstallable(pkg, (err) => {
let installable = !err
addBundled(pkg, (bundleErr) => {
var parent = earliestInstallable(tree, tree, pkg, log, null) || tree
var parent = earliestInstallable(tree, tree, pkg, log) || tree
var isLink = pkg._requested.type === 'directory'
var child = createChild({
package: pkg,
Expand Down Expand Up @@ -755,11 +755,11 @@ function preserveSymlinks () {
// Find the highest level in the tree that we can install this module in.
// If the module isn't installed above us yet, that'd be the very top.
// If it is, then it's the level below where its installed.
var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log, ignoreChild) {
validate('OOOOZ|OOOOO', arguments)
var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log) {
validate('OOOO', arguments)

function undeletedModuleMatches (child) {
return child !== ignoreChild && !child.removed && moduleName(child) === pkg.name
return !child.removed && moduleName(child) === pkg.name
}
const undeletedMatches = tree.children.filter(undeletedModuleMatches)
if (undeletedMatches.length) {
Expand All @@ -778,7 +778,7 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr
// If any of the children of this tree have conflicting
// binaries then we need to decline to install this package here.
var binaryMatches = pkg.bin && tree.children.some(function (child) {
if (child === ignoreChild || child.removed || !child.package.bin) return false
if (child.removed || !child.package.bin) return false
return Object.keys(child.package.bin).some(function (bin) {
return pkg.bin[bin]
})
Expand All @@ -804,23 +804,6 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr

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

// if any of the peer dependencies is a 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 dependencies are hoisted to another location yet, as they
// may not be loaded into the tree yet.
// We can ignore dev deps here as they are only installed on top-level
// and we can't hoist further than that anyway.
var peerDeps = pkg.peerDependencies
if (peerDeps) {
if (Object.keys(peerDeps).some(function (name) {
return deps[name]
})) {
return tree
}
}

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

Expand All @@ -829,5 +812,5 @@ var earliestInstallable = exports.earliestInstallable = function (requiredBy, tr

if (!preserveSymlinks() && /^[.][.][\\/]/.test(path.relative(tree.parent.realpath, tree.realpath))) return tree

return (earliestInstallable(requiredBy, tree.parent, pkg, log, ignoreChild) || tree)
return (earliestInstallable(requiredBy, tree.parent, pkg, log) || tree)
}
130 changes: 0 additions & 130 deletions test/tap/hoist-peer-dependencies.js

This file was deleted.

55 changes: 2 additions & 53 deletions test/tap/unit-deps-earliestInstallable.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ test('earliestInstallable should consider devDependencies', function (t) {
dep2a.parent = dep1
dep2.parent = pkg

var earliest = earliestInstallable(dep1, dep1, dep2a.package, log, null)
var earliest = earliestInstallable(dep1, dep1, dep2a.package, log)
t.isDeeply(earliest, dep1, 'should hoist package when an incompatible devDependency is present')
t.end()
})
Expand Down Expand Up @@ -108,58 +108,7 @@ test('earliestInstallable should reuse shared prod/dev deps when they are identi
dep1.parent = pkg
dep2.parent = pkg

var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null)
var earliest = earliestInstallable(dep1, dep1, dep2.package, log)
t.isDeeply(earliest, pkg, 'should reuse identical shared dev/prod deps when installing both')
t.end()
})

test('earliestInstallable should consider peerDependencies', function (t) {
var dep1 = {
children: [],
package: {
name: 'dep1',
dependencies: {
dep2: '1.0.0',
dep3: '1.0.0'
}
},
path: '/dep1',
realpath: '/dep1'
}

var dep2 = {
package: {
name: 'dep2',
version: '1.0.0',
peerDependencies: {
dep3: '1.0.0'
},
_requested: npa('dep2@^1.0.0')
},
parent: dep1,
path: '/dep1/node_modules/dep2',
realpath: '/dep1/node_modules/dep2'
}

var pkg = {
isTop: true,
children: [dep1],
package: {
name: 'pkg',
dependencies: { dep1: '1.0.0' }
},
path: '/',
realpath: '/'
}

dep1.parent = pkg

var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null)
t.isDeeply(earliest, dep1, 'should not be able to hoist the package to top-level')

dep1.children.push(dep2)

var earliestWithIgnore = earliestInstallable(dep1, dep1, dep2.package, log, dep2)
t.isDeeply(earliestWithIgnore, dep1, 'should not be able to hoist the package to top-level')
t.end()
})