Skip to content

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

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

Merged
aeschright 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 all 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
5 changes: 3 additions & 2 deletions lib/dedupe.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -142,8 +142,9 @@ 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
var hoistTo = earliestInstallable(tree, tree, child.package, log, child)
if (hoistTo && hoistTo !== tree) {
move(child, hoistTo, diff)
chain([
[andComputeMetadata(hoistTo)],
Expand Down
29 changes: 23 additions & 6 deletions lib/install/deps.js
View file
Open in desktop
Original file line numberDiff line numberDiff 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) || tree
var parent = earliestInstallable(tree, tree, pkg, log, null) || tree
var isLink = pkg._requested.type === 'directory'
var child = createChild({
package: pkg,
Expand DownExpand 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) {
validate('OOOO', arguments)
var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log, ignoreChild) {
validate('OOOOZ|OOOOO', arguments)

function undeletedModuleMatches (child) {
return !child.removed && moduleName(child) === pkg.name
return child !== ignoreChild && !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.removed || !child.package.bin) return false
if (child === ignoreChild || child.removed || !child.package.bin) return false
return Object.keys(child.package.bin).some(function (bin) {
return pkg.bin[bin]
})
Expand All@@ -804,6 +804,23 @@ 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@@ -812,5 +829,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) || tree)
return (earliestInstallable(requiredBy, tree.parent, pkg, log, ignoreChild) || tree)
}
2 changes: 1 addition & 1 deletion scripts/maketest
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -69,7 +69,7 @@ test('setup', t => {
})

test('example', t => {
return common.npm(['install'], conf).then((code, stdout, stderr) => {
return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
t.is(code, 0, 'command ran ok')
t.comment(stdout.trim())
t.comment(stderr.trim())
Expand Down
130 changes: 130 additions & 0 deletions test/tap/hoist-peer-dependencies.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
'use strict'
const path = require('path')
const fs = require('fs')
const test = require('tap').test
const Tacks = require('tacks')
const File = Tacks.File
const Dir = Tacks.Dir
const common = require('../common-tap.js')

const basedir = path.join(__dirname, path.basename(__filename, '.js'))
const testdir = path.join(basedir, 'testdir')
const cachedir = path.join(basedir, 'cache')
const globaldir = path.join(basedir, 'global')
const tmpdir = path.join(basedir, 'tmp')

const conf = {
cwd: testdir,
env: common.newEnv().extend({
npm_config_cache: cachedir,
npm_config_tmp: tmpdir,
npm_config_prefix: globaldir,
npm_config_registry: common.registry,
npm_config_loglevel: 'warn'
})
}

const fixture = new Tacks(Dir({
cache: Dir(),
global: Dir(),
tmp: Dir(),
testdir: Dir({
package: Dir({
'package.json': File({
name: 'package',
version: '1.0.0',
dependencies: {
'base-dep': '../packages/base-dep-1.0.0.tgz',
'plugin-dep': '../packages/plugin-dep-1.0.0.tgz'
}
})
}),
'package.json': File({
version: '1.0.0',
dependencies: {
package: 'file:./package',
'base-dep': './packages/base-dep-2.0.0.tgz'
}
}),
// file: dependencies do not work as they are symlinked and behave
// differently. Instead installation from tgz files is used.
packages: Dir({
'base-dep-1.0.0.tgz': File(Buffer.from(
'1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
'06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
'4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781240cf50cf4' +
'0c94b86ab906dab9a360148c8251300aa80400a44d97d100080000',
'hex'
)),
'base-dep-2.0.0.tgz': File(Buffer.from(
'1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
'06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
'4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781248cf40cf4' +
'0c94b86ab906dab9a360148c8251300aa80400aebbeeba00080000',
'hex'
)),
'plugin-dep-1.0.0.tgz': File(Buffer.from(
'1f8b080000000000000aed8f3d0e83300c8599394594b904476d3274e622' +
'295888fe8488408756dcbd0e513bb115a9aa946f79ce7bb1653b535f4c8b' +
'a58b2acebeb7d9c60080d69aadf90119b2bdd220a98203cba8504a916ebd' +
'c81a931fcd40ab7c3b27dec23efa273c73c6b83537e447c6dd756a3b5b34' +
'e8f82ef8771c7cd7db10490102a2eb10870a1dda066ddda1a7384ca1e464' +
'3c2eddd42044f97e164bb318db07a77f733ee7bfbe3a914824122f4e04e9' +
'2e00080000',
'hex'
))
})
})
}))

function setup () {
cleanup()
fixture.create(basedir)
}

function cleanup () {
fixture.remove(basedir)
}

test('setup', t => {
setup()
return common.fakeRegistry.listen()
})

test('example', t => {
return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
t.is(code, 0, 'command ran ok')
t.comment(stdout.trim())
t.comment(stderr.trim())
t.ok(fs.existsSync(path.join(testdir, 'node_modules')), 'did install')
var packageLock = JSON.parse(fs.readFileSync(path.join(testdir, 'package-lock.json'), 'utf8'))
t.similar(packageLock, {
dependencies: {
'base-dep': {
version: 'file:packages/base-dep-2.0.0.tgz'
},
'package': {
version: 'file:package',
dependencies: {
'base-dep': {
version: 'file:packages/base-dep-1.0.0.tgz'
},
// plugin-dep must not placed on top-level
'plugin-dep': {
version: 'file:packages/plugin-dep-1.0.0.tgz'
}
}
}
}
}, 'locks dependencies as unhoisted')
t.similar(Object.keys(packageLock.dependencies), ['base-dep', 'package'], 'has correct packages on top-level')
})
})

test('cleanup', t => {
common.fakeRegistry.close()
cleanup()
t.done()
})
55 changes: 53 additions & 2 deletions test/tap/unit-deps-earliestInstallable.js
View file
Open in desktop
Original file line numberDiff line numberDiff 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)
var earliest = earliestInstallable(dep1, dep1, dep2a.package, log, null)
t.isDeeply(earliest, dep1, 'should hoist package when an incompatible devDependency is present')
t.end()
})
Expand DownExpand Up@@ -108,7 +108,58 @@ 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)
var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null)
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()
})
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all 
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
install/dedupe: fix hoisting of packages with peerDeps by sokra · Pull Request #147 · npm/cli · GitHub
Skip to content
install/dedupe: fix hoisting of packages with peerDeps#147

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

 Merged

aeschright
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 all 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


5 changes: 3 additions & 2 deletions
lib/dedupe.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -142,8 +142,9 @@ 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
 var hoistTo = earliestInstallable(tree, tree, child.package, log, child)
 if (hoistTo && hoistTo !== tree) {
 move(child, hoistTo, diff)
 chain([
 [andComputeMetadata(hoistTo)],
Expand Down

29 changes: 23 additions & 6 deletions
lib/install/deps.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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) || tree
 var parent = earliestInstallable(tree, tree, pkg, log, null) || tree
 var isLink = pkg._requested.type === 'directory'
 var child = createChild({
 package: pkg,
Expand DownExpand 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) {
 validate('OOOO', arguments)
var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log, ignoreChild) {
 validate('OOOOZ|OOOOO', arguments)

 function undeletedModuleMatches (child) {
 return !child.removed && moduleName(child) === pkg.name
 return child !== ignoreChild && !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.removed || !child.package.bin) return false
 if (child === ignoreChild || child.removed || !child.package.bin) return false
 return Object.keys(child.package.bin).some(function (bin) {
 return pkg.bin[bin]
 })
Expand All@@ -804,6 +804,23 @@ 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@@ -812,5 +829,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) || tree)
 return (earliestInstallable(requiredBy, tree.parent, pkg, log, ignoreChild) || tree)
}

2 changes: 1 addition & 1 deletion
scripts/maketest

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -69,7 +69,7 @@ test('setup', t => {
})

test('example', t => {
 return common.npm(['install'], conf).then((code, stdout, stderr) => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
Expand Down

130 changes: 130 additions & 0 deletions
test/tap/hoist-peer-dependencies.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
'use strict'
const path = require('path')
const fs = require('fs')
const test = require('tap').test
const Tacks = require('tacks')
const File = Tacks.File
const Dir = Tacks.Dir
const common = require('../common-tap.js')

const basedir = path.join(__dirname, path.basename(__filename, '.js'))
const testdir = path.join(basedir, 'testdir')
const cachedir = path.join(basedir, 'cache')
const globaldir = path.join(basedir, 'global')
const tmpdir = path.join(basedir, 'tmp')

const conf = {
 cwd: testdir,
 env: common.newEnv().extend({
 npm_config_cache: cachedir,
 npm_config_tmp: tmpdir,
 npm_config_prefix: globaldir,
 npm_config_registry: common.registry,
 npm_config_loglevel: 'warn'
 })
}

const fixture = new Tacks(Dir({
 cache: Dir(),
 global: Dir(),
 tmp: Dir(),
 testdir: Dir({
 package: Dir({
 'package.json': File({
 name: 'package',
 version: '1.0.0',
 dependencies: {
 'base-dep': '../packages/base-dep-1.0.0.tgz',
 'plugin-dep': '../packages/plugin-dep-1.0.0.tgz'
 }
 })
 }),
 'package.json': File({
 version: '1.0.0',
 dependencies: {
 package: 'file:./package',
 'base-dep': './packages/base-dep-2.0.0.tgz'
 }
 }),
 // file: dependencies do not work as they are symlinked and behave
 // differently. Instead installation from tgz files is used.
 packages: Dir({
 'base-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781240cf50cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400a44d97d100080000',
 'hex'
 )),
 'base-dep-2.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781248cf40cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400aebbeeba00080000',
 'hex'
 )),
 'plugin-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000aed8f3d0e83300c8599394594b904476d3274e622' +
 '295888fe8488408756dcbd0e513bb115a9aa946f79ce7bb1653b535f4c8b' +
 'a58b2acebeb7d9c60080d69aadf90119b2bdd220a98203cba8504a916ebd' +
 'c81a931fcd40ab7c3b27dec23efa273c73c6b83537e447c6dd756a3b5b34' +
 'e8f82ef8771c7cd7db10490102a2eb10870a1dda066ddda1a7384ca1e464' +
 '3c2eddd42044f97e164bb318db07a77f733ee7bfbe3a914824122f4e04e9' +
 '2e00080000',
 'hex'
 ))
 })
 })
}))

function setup () {
 cleanup()
 fixture.create(basedir)
}

function cleanup () {
 fixture.remove(basedir)
}

test('setup', t => {
 setup()
 return common.fakeRegistry.listen()
})

test('example', t => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
 t.ok(fs.existsSync(path.join(testdir, 'node_modules')), 'did install')
 var packageLock = JSON.parse(fs.readFileSync(path.join(testdir, 'package-lock.json'), 'utf8'))
 t.similar(packageLock, {
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-2.0.0.tgz'
 },
 'package': {
 version: 'file:package',
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-1.0.0.tgz'
 },
 // plugin-dep must not placed on top-level
 'plugin-dep': {
 version: 'file:packages/plugin-dep-1.0.0.tgz'
 }
 }
 }
 }
 }, 'locks dependencies as unhoisted')
 t.similar(Object.keys(packageLock.dependencies), ['base-dep', 'package'], 'has correct packages on top-level')
 })
})

test('cleanup', t => {
 common.fakeRegistry.close()
 cleanup()
 t.done()
})

55 changes: 53 additions & 2 deletions
test/tap/unit-deps-earliestInstallable.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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)
 var earliest = earliestInstallable(dep1, dep1, dep2a.package, log, null)
 t.isDeeply(earliest, dep1, 'should hoist package when an incompatible devDependency is present')
 t.end()
})
Expand DownExpand Up@@ -108,7 +108,58 @@ 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)
 var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null)
 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()
})

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Force GitHub README to respect dark mode
(function() {
var style = document.createElement('style');
style.textContent = '
.markdown-body {
color-scheme: dark light;
}
.markdown-body pre { background: #161b22 !important; }
.markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }
.markdown-body table th, .markdown-body table td { border-color: #30363d !important; }
.markdown-body img { background: #0d1117; }
.markdown-body blockquote { border-left-color: #8b949e; }
.markdown-body hr { border-color: #30363d; }
';
document.head.appendChild(style);
})();
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
install/dedupe: fix hoisting of packages with peerDeps by sokra · Pull Request #147 · npm/cli · GitHub
Skip to content
install/dedupe: fix hoisting of packages with peerDeps#147

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

 Merged

aeschright
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 all 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


5 changes: 3 additions & 2 deletions
lib/dedupe.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -142,8 +142,9 @@ 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
 var hoistTo = earliestInstallable(tree, tree, child.package, log, child)
 if (hoistTo && hoistTo !== tree) {
 move(child, hoistTo, diff)
 chain([
 [andComputeMetadata(hoistTo)],
Expand Down

29 changes: 23 additions & 6 deletions
lib/install/deps.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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) || tree
 var parent = earliestInstallable(tree, tree, pkg, log, null) || tree
 var isLink = pkg._requested.type === 'directory'
 var child = createChild({
 package: pkg,
Expand DownExpand 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) {
 validate('OOOO', arguments)
var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log, ignoreChild) {
 validate('OOOOZ|OOOOO', arguments)

 function undeletedModuleMatches (child) {
 return !child.removed && moduleName(child) === pkg.name
 return child !== ignoreChild && !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.removed || !child.package.bin) return false
 if (child === ignoreChild || child.removed || !child.package.bin) return false
 return Object.keys(child.package.bin).some(function (bin) {
 return pkg.bin[bin]
 })
Expand All@@ -804,6 +804,23 @@ 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@@ -812,5 +829,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) || tree)
 return (earliestInstallable(requiredBy, tree.parent, pkg, log, ignoreChild) || tree)
}

2 changes: 1 addition & 1 deletion
scripts/maketest

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -69,7 +69,7 @@ test('setup', t => {
})

test('example', t => {
 return common.npm(['install'], conf).then((code, stdout, stderr) => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
Expand Down

130 changes: 130 additions & 0 deletions
test/tap/hoist-peer-dependencies.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
'use strict'
const path = require('path')
const fs = require('fs')
const test = require('tap').test
const Tacks = require('tacks')
const File = Tacks.File
const Dir = Tacks.Dir
const common = require('../common-tap.js')

const basedir = path.join(__dirname, path.basename(__filename, '.js'))
const testdir = path.join(basedir, 'testdir')
const cachedir = path.join(basedir, 'cache')
const globaldir = path.join(basedir, 'global')
const tmpdir = path.join(basedir, 'tmp')

const conf = {
 cwd: testdir,
 env: common.newEnv().extend({
 npm_config_cache: cachedir,
 npm_config_tmp: tmpdir,
 npm_config_prefix: globaldir,
 npm_config_registry: common.registry,
 npm_config_loglevel: 'warn'
 })
}

const fixture = new Tacks(Dir({
 cache: Dir(),
 global: Dir(),
 tmp: Dir(),
 testdir: Dir({
 package: Dir({
 'package.json': File({
 name: 'package',
 version: '1.0.0',
 dependencies: {
 'base-dep': '../packages/base-dep-1.0.0.tgz',
 'plugin-dep': '../packages/plugin-dep-1.0.0.tgz'
 }
 })
 }),
 'package.json': File({
 version: '1.0.0',
 dependencies: {
 package: 'file:./package',
 'base-dep': './packages/base-dep-2.0.0.tgz'
 }
 }),
 // file: dependencies do not work as they are symlinked and behave
 // differently. Instead installation from tgz files is used.
 packages: Dir({
 'base-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781240cf50cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400a44d97d100080000',
 'hex'
 )),
 'base-dep-2.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781248cf40cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400aebbeeba00080000',
 'hex'
 )),
 'plugin-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000aed8f3d0e83300c8599394594b904476d3274e622' +
 '295888fe8488408756dcbd0e513bb115a9aa946f79ce7bb1653b535f4c8b' +
 'a58b2acebeb7d9c60080d69aadf90119b2bdd220a98203cba8504a916ebd' +
 'c81a931fcd40ab7c3b27dec23efa273c73c6b83537e447c6dd756a3b5b34' +
 'e8f82ef8771c7cd7db10490102a2eb10870a1dda066ddda1a7384ca1e464' +
 '3c2eddd42044f97e164bb318db07a77f733ee7bfbe3a914824122f4e04e9' +
 '2e00080000',
 'hex'
 ))
 })
 })
}))

function setup () {
 cleanup()
 fixture.create(basedir)
}

function cleanup () {
 fixture.remove(basedir)
}

test('setup', t => {
 setup()
 return common.fakeRegistry.listen()
})

test('example', t => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
 t.ok(fs.existsSync(path.join(testdir, 'node_modules')), 'did install')
 var packageLock = JSON.parse(fs.readFileSync(path.join(testdir, 'package-lock.json'), 'utf8'))
 t.similar(packageLock, {
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-2.0.0.tgz'
 },
 'package': {
 version: 'file:package',
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-1.0.0.tgz'
 },
 // plugin-dep must not placed on top-level
 'plugin-dep': {
 version: 'file:packages/plugin-dep-1.0.0.tgz'
 }
 }
 }
 }
 }, 'locks dependencies as unhoisted')
 t.similar(Object.keys(packageLock.dependencies), ['base-dep', 'package'], 'has correct packages on top-level')
 })
})

test('cleanup', t => {
 common.fakeRegistry.close()
 cleanup()
 t.done()
})

55 changes: 53 additions & 2 deletions
test/tap/unit-deps-earliestInstallable.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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)
 var earliest = earliestInstallable(dep1, dep1, dep2a.package, log, null)
 t.isDeeply(earliest, dep1, 'should hoist package when an incompatible devDependency is present')
 t.end()
})
Expand DownExpand Up@@ -108,7 +108,58 @@ 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)
 var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null)
 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()
})

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Highlight search terms from Google/DuckDuckGo/Bing referrer
(function() {
var ref = document.referrer;
var terms = [];
if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {
var url = new URL(ref);
var q = url.searchParams.get('q') || url.searchParams.get('p');
if (q) {
terms = q.split(/\s+/).filter(function(t) { return t.length > 2; });
}
}
if (terms.length === 0) return;
var style = document.createElement('style');
style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';
document.head.appendChild(style);
function highlight(node) {
if (node.nodeType === 3) { // text node
var text = node.textContent;
var found = false;
terms.forEach(function(term) {
var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi');
if (regex.test(text)) {
found = true;
var frag = document.createDocumentFragment();
var parts = text.split(regex);
parts.forEach(function(part, i) {
if (i % 2 === 0) {
frag.appendChild(document.createTextNode(part));
} else {
var span = document.createElement('span');
span.className = 'userscript-highlight';
span.textContent = part;
frag.appendChild(span);
}
});
node.parentNode.replaceChild(frag, node);
}
});
} else if (node.nodeType === 1 && node.childNodes) { // element
var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];
if (!skipTags.includes(node.tagName)) {
Array.from(node.childNodes).forEach(highlight);
}
}
}
highlight(document.body);
// Re-highlight on dynamic content
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1 || node.nodeType === 3) highlight(node);
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
install/dedupe: fix hoisting of packages with peerDeps by sokra · Pull Request #147 · npm/cli · GitHub
Skip to content
install/dedupe: fix hoisting of packages with peerDeps#147

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

 Merged

aeschright
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 all 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


5 changes: 3 additions & 2 deletions
lib/dedupe.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -142,8 +142,9 @@ 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
 var hoistTo = earliestInstallable(tree, tree, child.package, log, child)
 if (hoistTo && hoistTo !== tree) {
 move(child, hoistTo, diff)
 chain([
 [andComputeMetadata(hoistTo)],
Expand Down

29 changes: 23 additions & 6 deletions
lib/install/deps.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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) || tree
 var parent = earliestInstallable(tree, tree, pkg, log, null) || tree
 var isLink = pkg._requested.type === 'directory'
 var child = createChild({
 package: pkg,
Expand DownExpand 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) {
 validate('OOOO', arguments)
var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log, ignoreChild) {
 validate('OOOOZ|OOOOO', arguments)

 function undeletedModuleMatches (child) {
 return !child.removed && moduleName(child) === pkg.name
 return child !== ignoreChild && !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.removed || !child.package.bin) return false
 if (child === ignoreChild || child.removed || !child.package.bin) return false
 return Object.keys(child.package.bin).some(function (bin) {
 return pkg.bin[bin]
 })
Expand All@@ -804,6 +804,23 @@ 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@@ -812,5 +829,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) || tree)
 return (earliestInstallable(requiredBy, tree.parent, pkg, log, ignoreChild) || tree)
}

2 changes: 1 addition & 1 deletion
scripts/maketest

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -69,7 +69,7 @@ test('setup', t => {
})

test('example', t => {
 return common.npm(['install'], conf).then((code, stdout, stderr) => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
Expand Down

130 changes: 130 additions & 0 deletions
test/tap/hoist-peer-dependencies.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
'use strict'
const path = require('path')
const fs = require('fs')
const test = require('tap').test
const Tacks = require('tacks')
const File = Tacks.File
const Dir = Tacks.Dir
const common = require('../common-tap.js')

const basedir = path.join(__dirname, path.basename(__filename, '.js'))
const testdir = path.join(basedir, 'testdir')
const cachedir = path.join(basedir, 'cache')
const globaldir = path.join(basedir, 'global')
const tmpdir = path.join(basedir, 'tmp')

const conf = {
 cwd: testdir,
 env: common.newEnv().extend({
 npm_config_cache: cachedir,
 npm_config_tmp: tmpdir,
 npm_config_prefix: globaldir,
 npm_config_registry: common.registry,
 npm_config_loglevel: 'warn'
 })
}

const fixture = new Tacks(Dir({
 cache: Dir(),
 global: Dir(),
 tmp: Dir(),
 testdir: Dir({
 package: Dir({
 'package.json': File({
 name: 'package',
 version: '1.0.0',
 dependencies: {
 'base-dep': '../packages/base-dep-1.0.0.tgz',
 'plugin-dep': '../packages/plugin-dep-1.0.0.tgz'
 }
 })
 }),
 'package.json': File({
 version: '1.0.0',
 dependencies: {
 package: 'file:./package',
 'base-dep': './packages/base-dep-2.0.0.tgz'
 }
 }),
 // file: dependencies do not work as they are symlinked and behave
 // differently. Instead installation from tgz files is used.
 packages: Dir({
 'base-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781240cf50cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400a44d97d100080000',
 'hex'
 )),
 'base-dep-2.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781248cf40cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400aebbeeba00080000',
 'hex'
 )),
 'plugin-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000aed8f3d0e83300c8599394594b904476d3274e622' +
 '295888fe8488408756dcbd0e513bb115a9aa946f79ce7bb1653b535f4c8b' +
 'a58b2acebeb7d9c60080d69aadf90119b2bdd220a98203cba8504a916ebd' +
 'c81a931fcd40ab7c3b27dec23efa273c73c6b83537e447c6dd756a3b5b34' +
 'e8f82ef8771c7cd7db10490102a2eb10870a1dda066ddda1a7384ca1e464' +
 '3c2eddd42044f97e164bb318db07a77f733ee7bfbe3a914824122f4e04e9' +
 '2e00080000',
 'hex'
 ))
 })
 })
}))

function setup () {
 cleanup()
 fixture.create(basedir)
}

function cleanup () {
 fixture.remove(basedir)
}

test('setup', t => {
 setup()
 return common.fakeRegistry.listen()
})

test('example', t => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
 t.ok(fs.existsSync(path.join(testdir, 'node_modules')), 'did install')
 var packageLock = JSON.parse(fs.readFileSync(path.join(testdir, 'package-lock.json'), 'utf8'))
 t.similar(packageLock, {
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-2.0.0.tgz'
 },
 'package': {
 version: 'file:package',
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-1.0.0.tgz'
 },
 // plugin-dep must not placed on top-level
 'plugin-dep': {
 version: 'file:packages/plugin-dep-1.0.0.tgz'
 }
 }
 }
 }
 }, 'locks dependencies as unhoisted')
 t.similar(Object.keys(packageLock.dependencies), ['base-dep', 'package'], 'has correct packages on top-level')
 })
})

test('cleanup', t => {
 common.fakeRegistry.close()
 cleanup()
 t.done()
})

55 changes: 53 additions & 2 deletions
test/tap/unit-deps-earliestInstallable.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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)
 var earliest = earliestInstallable(dep1, dep1, dep2a.package, log, null)
 t.isDeeply(earliest, dep1, 'should hoist package when an incompatible devDependency is present')
 t.end()
})
Expand DownExpand Up@@ -108,7 +108,58 @@ 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)
 var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null)
 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()
})

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Strip utm_, fbclid, gclid, etc. from all links on page
(function() {
var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',
'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',
'ref', 'ref_src', 'source', 'medium', 'campaign'];
function cleanUrl(url) {
try {
var u = new URL(url, window.location.origin);
var changed = false;
trackingParams.forEach(function(p) {
if (u.searchParams.has(p)) {
u.searchParams.delete(p);
changed = true;
}
});
return changed ? u.toString() : url;
} catch (e) {
return url;
}
}
function cleanLinks() {
document.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
cleanLinks();
var observer = new MutationObserver(function(mutations) {
mutations.forEach(function(m) {
m.addedNodes.forEach(function(node) {
if (node.nodeType === 1) {
if (node.tagName === 'A') cleanLinks();
node.querySelectorAll('a[href]').forEach(function(a) {
var clean = cleanUrl(a.href);
if (clean !== a.href) a.href = clean;
});
}
});
});
});
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '
install/dedupe: fix hoisting of packages with peerDeps by sokra · Pull Request #147 · npm/cli · GitHub
Skip to content
install/dedupe: fix hoisting of packages with peerDeps#147

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

 Merged

aeschright
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 all 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


5 changes: 3 additions & 2 deletions
lib/dedupe.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -142,8 +142,9 @@ 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
 var hoistTo = earliestInstallable(tree, tree, child.package, log, child)
 if (hoistTo && hoistTo !== tree) {
 move(child, hoistTo, diff)
 chain([
 [andComputeMetadata(hoistTo)],
Expand Down

29 changes: 23 additions & 6 deletions
lib/install/deps.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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) || tree
 var parent = earliestInstallable(tree, tree, pkg, log, null) || tree
 var isLink = pkg._requested.type === 'directory'
 var child = createChild({
 package: pkg,
Expand DownExpand 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) {
 validate('OOOO', arguments)
var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log, ignoreChild) {
 validate('OOOOZ|OOOOO', arguments)

 function undeletedModuleMatches (child) {
 return !child.removed && moduleName(child) === pkg.name
 return child !== ignoreChild && !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.removed || !child.package.bin) return false
 if (child === ignoreChild || child.removed || !child.package.bin) return false
 return Object.keys(child.package.bin).some(function (bin) {
 return pkg.bin[bin]
 })
Expand All@@ -804,6 +804,23 @@ 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@@ -812,5 +829,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) || tree)
 return (earliestInstallable(requiredBy, tree.parent, pkg, log, ignoreChild) || tree)
}

2 changes: 1 addition & 1 deletion
scripts/maketest

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -69,7 +69,7 @@ test('setup', t => {
})

test('example', t => {
 return common.npm(['install'], conf).then((code, stdout, stderr) => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
Expand Down

130 changes: 130 additions & 0 deletions
test/tap/hoist-peer-dependencies.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
'use strict'
const path = require('path')
const fs = require('fs')
const test = require('tap').test
const Tacks = require('tacks')
const File = Tacks.File
const Dir = Tacks.Dir
const common = require('../common-tap.js')

const basedir = path.join(__dirname, path.basename(__filename, '.js'))
const testdir = path.join(basedir, 'testdir')
const cachedir = path.join(basedir, 'cache')
const globaldir = path.join(basedir, 'global')
const tmpdir = path.join(basedir, 'tmp')

const conf = {
 cwd: testdir,
 env: common.newEnv().extend({
 npm_config_cache: cachedir,
 npm_config_tmp: tmpdir,
 npm_config_prefix: globaldir,
 npm_config_registry: common.registry,
 npm_config_loglevel: 'warn'
 })
}

const fixture = new Tacks(Dir({
 cache: Dir(),
 global: Dir(),
 tmp: Dir(),
 testdir: Dir({
 package: Dir({
 'package.json': File({
 name: 'package',
 version: '1.0.0',
 dependencies: {
 'base-dep': '../packages/base-dep-1.0.0.tgz',
 'plugin-dep': '../packages/plugin-dep-1.0.0.tgz'
 }
 })
 }),
 'package.json': File({
 version: '1.0.0',
 dependencies: {
 package: 'file:./package',
 'base-dep': './packages/base-dep-2.0.0.tgz'
 }
 }),
 // file: dependencies do not work as they are symlinked and behave
 // differently. Instead installation from tgz files is used.
 packages: Dir({
 'base-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781240cf50cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400a44d97d100080000',
 'hex'
 )),
 'base-dep-2.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781248cf40cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400aebbeeba00080000',
 'hex'
 )),
 'plugin-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000aed8f3d0e83300c8599394594b904476d3274e622' +
 '295888fe8488408756dcbd0e513bb115a9aa946f79ce7bb1653b535f4c8b' +
 'a58b2acebeb7d9c60080d69aadf90119b2bdd220a98203cba8504a916ebd' +
 'c81a931fcd40ab7c3b27dec23efa273c73c6b83537e447c6dd756a3b5b34' +
 'e8f82ef8771c7cd7db10490102a2eb10870a1dda066ddda1a7384ca1e464' +
 '3c2eddd42044f97e164bb318db07a77f733ee7bfbe3a914824122f4e04e9' +
 '2e00080000',
 'hex'
 ))
 })
 })
}))

function setup () {
 cleanup()
 fixture.create(basedir)
}

function cleanup () {
 fixture.remove(basedir)
}

test('setup', t => {
 setup()
 return common.fakeRegistry.listen()
})

test('example', t => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
 t.ok(fs.existsSync(path.join(testdir, 'node_modules')), 'did install')
 var packageLock = JSON.parse(fs.readFileSync(path.join(testdir, 'package-lock.json'), 'utf8'))
 t.similar(packageLock, {
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-2.0.0.tgz'
 },
 'package': {
 version: 'file:package',
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-1.0.0.tgz'
 },
 // plugin-dep must not placed on top-level
 'plugin-dep': {
 version: 'file:packages/plugin-dep-1.0.0.tgz'
 }
 }
 }
 }
 }, 'locks dependencies as unhoisted')
 t.similar(Object.keys(packageLock.dependencies), ['base-dep', 'package'], 'has correct packages on top-level')
 })
})

test('cleanup', t => {
 common.fakeRegistry.close()
 cleanup()
 t.done()
})

55 changes: 53 additions & 2 deletions
test/tap/unit-deps-earliestInstallable.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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)
 var earliest = earliestInstallable(dep1, dep1, dep2a.package, log, null)
 t.isDeeply(earliest, dep1, 'should hoist package when an incompatible devDependency is present')
 t.end()
})
Expand DownExpand Up@@ -108,7 +108,58 @@ 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)
 var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null)
 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()
})

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Auto-enable theater mode on YouTube
(function() {
function tryTheater() {
var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]');
if (btn && !btn.classList.contains('activated')) {
btn.click();
}
}
// Try immediately
tryTheater();
// Try after navigation (SPA)
var lastUrl = location.href;
setInterval(function() {
if (location.href !== lastUrl) {
lastUrl = location.href;
setTimeout(tryTheater, 500);
}
}, 1000);
// Also try on player load
var observer = new MutationObserver(tryTheater);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
install/dedupe: fix hoisting of packages with peerDeps by sokra · Pull Request #147 · npm/cli · GitHub
Skip to content
install/dedupe: fix hoisting of packages with peerDeps#147

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

 Merged

aeschright
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 all 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


5 changes: 3 additions & 2 deletions
lib/dedupe.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -142,8 +142,9 @@ 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
 var hoistTo = earliestInstallable(tree, tree, child.package, log, child)
 if (hoistTo && hoistTo !== tree) {
 move(child, hoistTo, diff)
 chain([
 [andComputeMetadata(hoistTo)],
Expand Down

29 changes: 23 additions & 6 deletions
lib/install/deps.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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) || tree
 var parent = earliestInstallable(tree, tree, pkg, log, null) || tree
 var isLink = pkg._requested.type === 'directory'
 var child = createChild({
 package: pkg,
Expand DownExpand 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) {
 validate('OOOO', arguments)
var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log, ignoreChild) {
 validate('OOOOZ|OOOOO', arguments)

 function undeletedModuleMatches (child) {
 return !child.removed && moduleName(child) === pkg.name
 return child !== ignoreChild && !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.removed || !child.package.bin) return false
 if (child === ignoreChild || child.removed || !child.package.bin) return false
 return Object.keys(child.package.bin).some(function (bin) {
 return pkg.bin[bin]
 })
Expand All@@ -804,6 +804,23 @@ 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@@ -812,5 +829,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) || tree)
 return (earliestInstallable(requiredBy, tree.parent, pkg, log, ignoreChild) || tree)
}

2 changes: 1 addition & 1 deletion
scripts/maketest

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -69,7 +69,7 @@ test('setup', t => {
})

test('example', t => {
 return common.npm(['install'], conf).then((code, stdout, stderr) => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
Expand Down

130 changes: 130 additions & 0 deletions
test/tap/hoist-peer-dependencies.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
'use strict'
const path = require('path')
const fs = require('fs')
const test = require('tap').test
const Tacks = require('tacks')
const File = Tacks.File
const Dir = Tacks.Dir
const common = require('../common-tap.js')

const basedir = path.join(__dirname, path.basename(__filename, '.js'))
const testdir = path.join(basedir, 'testdir')
const cachedir = path.join(basedir, 'cache')
const globaldir = path.join(basedir, 'global')
const tmpdir = path.join(basedir, 'tmp')

const conf = {
 cwd: testdir,
 env: common.newEnv().extend({
 npm_config_cache: cachedir,
 npm_config_tmp: tmpdir,
 npm_config_prefix: globaldir,
 npm_config_registry: common.registry,
 npm_config_loglevel: 'warn'
 })
}

const fixture = new Tacks(Dir({
 cache: Dir(),
 global: Dir(),
 tmp: Dir(),
 testdir: Dir({
 package: Dir({
 'package.json': File({
 name: 'package',
 version: '1.0.0',
 dependencies: {
 'base-dep': '../packages/base-dep-1.0.0.tgz',
 'plugin-dep': '../packages/plugin-dep-1.0.0.tgz'
 }
 })
 }),
 'package.json': File({
 version: '1.0.0',
 dependencies: {
 package: 'file:./package',
 'base-dep': './packages/base-dep-2.0.0.tgz'
 }
 }),
 // file: dependencies do not work as they are symlinked and behave
 // differently. Instead installation from tgz files is used.
 packages: Dir({
 'base-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781240cf50cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400a44d97d100080000',
 'hex'
 )),
 'base-dep-2.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781248cf40cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400aebbeeba00080000',
 'hex'
 )),
 'plugin-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000aed8f3d0e83300c8599394594b904476d3274e622' +
 '295888fe8488408756dcbd0e513bb115a9aa946f79ce7bb1653b535f4c8b' +
 'a58b2acebeb7d9c60080d69aadf90119b2bdd220a98203cba8504a916ebd' +
 'c81a931fcd40ab7c3b27dec23efa273c73c6b83537e447c6dd756a3b5b34' +
 'e8f82ef8771c7cd7db10490102a2eb10870a1dda066ddda1a7384ca1e464' +
 '3c2eddd42044f97e164bb318db07a77f733ee7bfbe3a914824122f4e04e9' +
 '2e00080000',
 'hex'
 ))
 })
 })
}))

function setup () {
 cleanup()
 fixture.create(basedir)
}

function cleanup () {
 fixture.remove(basedir)
}

test('setup', t => {
 setup()
 return common.fakeRegistry.listen()
})

test('example', t => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
 t.ok(fs.existsSync(path.join(testdir, 'node_modules')), 'did install')
 var packageLock = JSON.parse(fs.readFileSync(path.join(testdir, 'package-lock.json'), 'utf8'))
 t.similar(packageLock, {
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-2.0.0.tgz'
 },
 'package': {
 version: 'file:package',
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-1.0.0.tgz'
 },
 // plugin-dep must not placed on top-level
 'plugin-dep': {
 version: 'file:packages/plugin-dep-1.0.0.tgz'
 }
 }
 }
 }
 }, 'locks dependencies as unhoisted')
 t.similar(Object.keys(packageLock.dependencies), ['base-dep', 'package'], 'has correct packages on top-level')
 })
})

test('cleanup', t => {
 common.fakeRegistry.close()
 cleanup()
 t.done()
})

55 changes: 53 additions & 2 deletions
test/tap/unit-deps-earliestInstallable.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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)
 var earliest = earliestInstallable(dep1, dep1, dep2a.package, log, null)
 t.isDeeply(earliest, dep1, 'should hoist package when an incompatible devDependency is present')
 t.end()
})
Expand DownExpand Up@@ -108,7 +108,58 @@ 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)
 var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null)
 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()
})

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Remove or un-stick sticky/fixed headers that block content
(function() {
function unstick() {
document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) {
if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {
el.style.position = 'static';
el.style.top = 'auto';
el.style.zIndex = 'auto';
}
});
}
unstick();
var observer = new MutationObserver(unstick);
observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });
})();
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '
install/dedupe: fix hoisting of packages with peerDeps by sokra · Pull Request #147 · npm/cli · GitHub
Skip to content
install/dedupe: fix hoisting of packages with peerDeps#147

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

 Merged

aeschright
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 all 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


5 changes: 3 additions & 2 deletions
lib/dedupe.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -142,8 +142,9 @@ 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
 var hoistTo = earliestInstallable(tree, tree, child.package, log, child)
 if (hoistTo && hoistTo !== tree) {
 move(child, hoistTo, diff)
 chain([
 [andComputeMetadata(hoistTo)],
Expand Down

29 changes: 23 additions & 6 deletions
lib/install/deps.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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) || tree
 var parent = earliestInstallable(tree, tree, pkg, log, null) || tree
 var isLink = pkg._requested.type === 'directory'
 var child = createChild({
 package: pkg,
Expand DownExpand 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) {
 validate('OOOO', arguments)
var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log, ignoreChild) {
 validate('OOOOZ|OOOOO', arguments)

 function undeletedModuleMatches (child) {
 return !child.removed && moduleName(child) === pkg.name
 return child !== ignoreChild && !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.removed || !child.package.bin) return false
 if (child === ignoreChild || child.removed || !child.package.bin) return false
 return Object.keys(child.package.bin).some(function (bin) {
 return pkg.bin[bin]
 })
Expand All@@ -804,6 +804,23 @@ 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@@ -812,5 +829,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) || tree)
 return (earliestInstallable(requiredBy, tree.parent, pkg, log, ignoreChild) || tree)
}

2 changes: 1 addition & 1 deletion
scripts/maketest

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -69,7 +69,7 @@ test('setup', t => {
})

test('example', t => {
 return common.npm(['install'], conf).then((code, stdout, stderr) => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
Expand Down

130 changes: 130 additions & 0 deletions
test/tap/hoist-peer-dependencies.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
'use strict'
const path = require('path')
const fs = require('fs')
const test = require('tap').test
const Tacks = require('tacks')
const File = Tacks.File
const Dir = Tacks.Dir
const common = require('../common-tap.js')

const basedir = path.join(__dirname, path.basename(__filename, '.js'))
const testdir = path.join(basedir, 'testdir')
const cachedir = path.join(basedir, 'cache')
const globaldir = path.join(basedir, 'global')
const tmpdir = path.join(basedir, 'tmp')

const conf = {
 cwd: testdir,
 env: common.newEnv().extend({
 npm_config_cache: cachedir,
 npm_config_tmp: tmpdir,
 npm_config_prefix: globaldir,
 npm_config_registry: common.registry,
 npm_config_loglevel: 'warn'
 })
}

const fixture = new Tacks(Dir({
 cache: Dir(),
 global: Dir(),
 tmp: Dir(),
 testdir: Dir({
 package: Dir({
 'package.json': File({
 name: 'package',
 version: '1.0.0',
 dependencies: {
 'base-dep': '../packages/base-dep-1.0.0.tgz',
 'plugin-dep': '../packages/plugin-dep-1.0.0.tgz'
 }
 })
 }),
 'package.json': File({
 version: '1.0.0',
 dependencies: {
 package: 'file:./package',
 'base-dep': './packages/base-dep-2.0.0.tgz'
 }
 }),
 // file: dependencies do not work as they are symlinked and behave
 // differently. Instead installation from tgz files is used.
 packages: Dir({
 'base-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781240cf50cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400a44d97d100080000',
 'hex'
 )),
 'base-dep-2.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781248cf40cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400aebbeeba00080000',
 'hex'
 )),
 'plugin-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000aed8f3d0e83300c8599394594b904476d3274e622' +
 '295888fe8488408756dcbd0e513bb115a9aa946f79ce7bb1653b535f4c8b' +
 'a58b2acebeb7d9c60080d69aadf90119b2bdd220a98203cba8504a916ebd' +
 'c81a931fcd40ab7c3b27dec23efa273c73c6b83537e447c6dd756a3b5b34' +
 'e8f82ef8771c7cd7db10490102a2eb10870a1dda066ddda1a7384ca1e464' +
 '3c2eddd42044f97e164bb318db07a77f733ee7bfbe3a914824122f4e04e9' +
 '2e00080000',
 'hex'
 ))
 })
 })
}))

function setup () {
 cleanup()
 fixture.create(basedir)
}

function cleanup () {
 fixture.remove(basedir)
}

test('setup', t => {
 setup()
 return common.fakeRegistry.listen()
})

test('example', t => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
 t.ok(fs.existsSync(path.join(testdir, 'node_modules')), 'did install')
 var packageLock = JSON.parse(fs.readFileSync(path.join(testdir, 'package-lock.json'), 'utf8'))
 t.similar(packageLock, {
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-2.0.0.tgz'
 },
 'package': {
 version: 'file:package',
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-1.0.0.tgz'
 },
 // plugin-dep must not placed on top-level
 'plugin-dep': {
 version: 'file:packages/plugin-dep-1.0.0.tgz'
 }
 }
 }
 }
 }, 'locks dependencies as unhoisted')
 t.similar(Object.keys(packageLock.dependencies), ['base-dep', 'package'], 'has correct packages on top-level')
 })
})

test('cleanup', t => {
 common.fakeRegistry.close()
 cleanup()
 t.done()
})

55 changes: 53 additions & 2 deletions
test/tap/unit-deps-earliestInstallable.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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)
 var earliest = earliestInstallable(dep1, dep1, dep2a.package, log, null)
 t.isDeeply(earliest, dep1, 'should hoist package when an incompatible devDependency is present')
 t.end()
})
Expand DownExpand Up@@ -108,7 +108,58 @@ 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)
 var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null)
 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()
})

, 'i');
if (__m === '*' || __re.test(location.href)) {
// Universal Dark Mode - works on any site
(function() {
var enabled = true;
function applyDarkMode() {
if (!enabled) return;
// Create style element if it doesn't exist
var style = document.getElementById('universal-dark-mode-style');
if (!style) {
style = document.createElement('style');
style.id = 'universal-dark-mode-style';
document.head.appendChild(style);
}
// Dark mode CSS - inverts colors but preserves images/video
style.textContent = '
/* Invert everything except media */
html {
filter: invert(1) hue-rotate(180deg) !important;
background: #1a1a2e !important;
}
/* Restore images, videos, iframes, canvas */
img, video, iframe, canvas, svg, picture, [style*="background-image"] {
filter: invert(1) hue-rotate(180deg) !important;
}
/* Preserve specific elements that should not be inverted */
.no-dark-mode, .no-dark-mode *,
[data-theme="light"], [data-theme="light"],
.ace_editor, .ace_editor *,
.CodeMirror, .CodeMirror *,
.monaco-editor, .monaco-editor *,
.markdown-body pre, .markdown-body pre *,
.highlight, .highlight *,
pre code, pre code * {
filter: none !important;
}
/* Fix common UI elements */
.modal, .popup, .dropdown-menu, .tooltip, .popover {
filter: invert(1) hue-rotate(180deg) !important;
background: #2d2d44 !important;
border-color: #444 !important;
}
/* Scrollbars */
::-webkit-scrollbar { background: #1a1a2e !important; }
::-webkit-scrollbar-thumb { background: #444 !important; }
::-webkit-scrollbar-thumb:hover { background: #555 !important; }
/* Selection */
::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }
';
}
function removeDarkMode() {
var style = document.getElementById('universal-dark-mode-style');
if (style) style.remove();
}
// Toggle with Alt+Shift+D
document.addEventListener('keydown', function(e) {
if (e.altKey && e.shiftKey && e.key === 'D') {
e.preventDefault();
enabled = !enabled;
if (enabled) {
applyDarkMode();
console.log('[Universal Dark Mode] Enabled');
} else {
removeDarkMode();
console.log('[Universal Dark Mode] Disabled');
}
}
});
// Apply on load
applyDarkMode();
// Re-apply on dynamic content
var observer = new MutationObserver(function(mutations) {
if (enabled && !document.getElementById('universal-dark-mode-style')) {
applyDarkMode();
}
});
observer.observe(document.head, { childList: true });
console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');
})();
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();
install/dedupe: fix hoisting of packages with peerDeps by sokra · Pull Request #147 · npm/cli · GitHub
Skip to content
install/dedupe: fix hoisting of packages with peerDeps#147

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

 Merged

aeschright
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 all 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


5 changes: 3 additions & 2 deletions
lib/dedupe.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -142,8 +142,9 @@ 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
 var hoistTo = earliestInstallable(tree, tree, child.package, log, child)
 if (hoistTo && hoistTo !== tree) {
 move(child, hoistTo, diff)
 chain([
 [andComputeMetadata(hoistTo)],
Expand Down

29 changes: 23 additions & 6 deletions
lib/install/deps.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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) || tree
 var parent = earliestInstallable(tree, tree, pkg, log, null) || tree
 var isLink = pkg._requested.type === 'directory'
 var child = createChild({
 package: pkg,
Expand DownExpand 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) {
 validate('OOOO', arguments)
var earliestInstallable = exports.earliestInstallable = function (requiredBy, tree, pkg, log, ignoreChild) {
 validate('OOOOZ|OOOOO', arguments)

 function undeletedModuleMatches (child) {
 return !child.removed && moduleName(child) === pkg.name
 return child !== ignoreChild && !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.removed || !child.package.bin) return false
 if (child === ignoreChild || child.removed || !child.package.bin) return false
 return Object.keys(child.package.bin).some(function (bin) {
 return pkg.bin[bin]
 })
Expand All@@ -804,6 +804,23 @@ 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@@ -812,5 +829,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) || tree)
 return (earliestInstallable(requiredBy, tree.parent, pkg, log, ignoreChild) || tree)
}

2 changes: 1 addition & 1 deletion
scripts/maketest

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -69,7 +69,7 @@ test('setup', t => {
})

test('example', t => {
 return common.npm(['install'], conf).then((code, stdout, stderr) => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
Expand Down

130 changes: 130 additions & 0 deletions
test/tap/hoist-peer-dependencies.js

View file


Open in desktop

Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
'use strict'
const path = require('path')
const fs = require('fs')
const test = require('tap').test
const Tacks = require('tacks')
const File = Tacks.File
const Dir = Tacks.Dir
const common = require('../common-tap.js')

const basedir = path.join(__dirname, path.basename(__filename, '.js'))
const testdir = path.join(basedir, 'testdir')
const cachedir = path.join(basedir, 'cache')
const globaldir = path.join(basedir, 'global')
const tmpdir = path.join(basedir, 'tmp')

const conf = {
 cwd: testdir,
 env: common.newEnv().extend({
 npm_config_cache: cachedir,
 npm_config_tmp: tmpdir,
 npm_config_prefix: globaldir,
 npm_config_registry: common.registry,
 npm_config_loglevel: 'warn'
 })
}

const fixture = new Tacks(Dir({
 cache: Dir(),
 global: Dir(),
 tmp: Dir(),
 testdir: Dir({
 package: Dir({
 'package.json': File({
 name: 'package',
 version: '1.0.0',
 dependencies: {
 'base-dep': '../packages/base-dep-1.0.0.tgz',
 'plugin-dep': '../packages/plugin-dep-1.0.0.tgz'
 }
 })
 }),
 'package.json': File({
 version: '1.0.0',
 dependencies: {
 package: 'file:./package',
 'base-dep': './packages/base-dep-2.0.0.tgz'
 }
 }),
 // file: dependencies do not work as they are symlinked and behave
 // differently. Instead installation from tgz files is used.
 packages: Dir({
 'base-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781240cf50cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400a44d97d100080000',
 'hex'
 )),
 'base-dep-2.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000a2b484cce4e4c4fd52f80d07a59c5f9790c540606' +
 '06066666660ad8c4c1c0d45c81c1d8d4ccc0d0d0ccccc0448101c8303505' +
 'd1d4760836505a5c925804740aa5e640bca200a78708a8e6525050ca4bcc' +
 '4d55b252504a4a2c4ed54d492d50d2018996a5161567e6e781248cf40cf4' +
 '0c94b86ab906dab9a360148c8251300aa80400aebbeeba00080000',
 'hex'
 )),
 'plugin-dep-1.0.0.tgz': File(Buffer.from(
 '1f8b080000000000000aed8f3d0e83300c8599394594b904476d3274e622' +
 '295888fe8488408756dcbd0e513bb115a9aa946f79ce7bb1653b535f4c8b' +
 'a58b2acebeb7d9c60080d69aadf90119b2bdd220a98203cba8504a916ebd' +
 'c81a931fcd40ab7c3b27dec23efa273c73c6b83537e447c6dd756a3b5b34' +
 'e8f82ef8771c7cd7db10490102a2eb10870a1dda066ddda1a7384ca1e464' +
 '3c2eddd42044f97e164bb318db07a77f733ee7bfbe3a914824122f4e04e9' +
 '2e00080000',
 'hex'
 ))
 })
 })
}))

function setup () {
 cleanup()
 fixture.create(basedir)
}

function cleanup () {
 fixture.remove(basedir)
}

test('setup', t => {
 setup()
 return common.fakeRegistry.listen()
})

test('example', t => {
 return common.npm(['install'], conf).then(([code, stdout, stderr]) => {
 t.is(code, 0, 'command ran ok')
 t.comment(stdout.trim())
 t.comment(stderr.trim())
 t.ok(fs.existsSync(path.join(testdir, 'node_modules')), 'did install')
 var packageLock = JSON.parse(fs.readFileSync(path.join(testdir, 'package-lock.json'), 'utf8'))
 t.similar(packageLock, {
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-2.0.0.tgz'
 },
 'package': {
 version: 'file:package',
 dependencies: {
 'base-dep': {
 version: 'file:packages/base-dep-1.0.0.tgz'
 },
 // plugin-dep must not placed on top-level
 'plugin-dep': {
 version: 'file:packages/plugin-dep-1.0.0.tgz'
 }
 }
 }
 }
 }, 'locks dependencies as unhoisted')
 t.similar(Object.keys(packageLock.dependencies), ['base-dep', 'package'], 'has correct packages on top-level')
 })
})

test('cleanup', t => {
 common.fakeRegistry.close()
 cleanup()
 t.done()
})

55 changes: 53 additions & 2 deletions
test/tap/unit-deps-earliestInstallable.js

View file


Open in desktop

Original file line numberDiff line numberDiff 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)
 var earliest = earliestInstallable(dep1, dep1, dep2a.package, log, null)
 t.isDeeply(earliest, dep1, 'should hoist package when an incompatible devDependency is present')
 t.end()
})
Expand DownExpand Up@@ -108,7 +108,58 @@ 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)
 var earliest = earliestInstallable(dep1, dep1, dep2.package, log, null)
 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()
})