From 45772af0ddca54b658cb2ba2182eec26d0a4729d Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 6 Aug 2019 11:49:32 -0700 Subject: [PATCH] install: do not descend into directory deps' child modules This builds on the work of https://github.com/npm/cli/pull/217, bringing back the logic in 2f0c883519f17c94411dd1d9877c5666f260c12f for all deps other than 'directory' symlinks where it causes problems. This also causes the installer to *fix* shrinkwraps that inappropriately list the dependencies of directory symlink packages, which goes further to address the problems highlighted in https://npm.community/t/installing-the-same-module-under-multiple-relative-paths-fails-on-linux/8863 and https://npm.community/t/reinstall-breaks-after-npm-update-to-6-10-2/9327, even if a prior npm install created a broken package-lock.json file. Related: https://github.com/npm/cli/pull/217 Credit: @isaacs Fix: https://npm.community/t/reinstall-breaks-after-npm-update-to-6-10-2/9327 Fix: https://npm.community/t/installing-the-same-module-under-multiple-relative-paths-fails-on-linux/8863 --- lib/install/inflate-shrinkwrap.js | 20 ++- test/tap/install-from-local-multipath.js | 6 +- .../install-symlink-leave-children-alone.js | 127 ++++++++++++++++++ 3 files changed, 146 insertions(+), 7 deletions(-) create mode 100644 test/tap/install-symlink-leave-children-alone.js diff --git a/lib/install/inflate-shrinkwrap.js b/lib/install/inflate-shrinkwrap.js index b0b71ef6b1323..9fb38167b84aa 100644 --- a/lib/install/inflate-shrinkwrap.js +++ b/lib/install/inflate-shrinkwrap.js @@ -50,8 +50,12 @@ function inflateShrinkwrap (topPath, tree, swdeps, opts) { return BB.each(Object.keys(swdeps), (name) => { const sw = swdeps[name] - const dependencies = sw.dependencies || {} const requested = realizeShrinkwrapSpecifier(name, sw, topPath) + // We should not muck about in the node_modules folder of + // symlinked packages. Treat its dependencies as if they + // were empty, since it's none of our business. + const dependencies = requested.type === 'directory' ? {} + : sw.dependencies || {} return inflatableChild( onDisk[name], name, topPath, tree, sw, requested, opts ).then((child) => { @@ -141,6 +145,10 @@ function isGit (sw) { } function makeFakeChild (name, topPath, tree, sw, requested) { + // We should not muck about in the node_modules folder of + // symlinked packages. Treat its dependencies as if they + // were empty, since it's none of our business. + const isDirectory = requested.type === 'directory' const from = sw.from || requested.raw const pkg = { name: name, @@ -156,7 +164,7 @@ function makeFakeChild (name, topPath, tree, sw, requested) { _spec: requested.rawSpec, _where: topPath, _args: [[requested.toString(), topPath]], - dependencies: sw.requires + dependencies: isDirectory ? {} : sw.requires } if (!sw.bundled) { @@ -167,16 +175,16 @@ function makeFakeChild (name, topPath, tree, sw, requested) { } const child = createChild({ package: pkg, - loaded: true, + loaded: isDirectory, parent: tree, children: [], fromShrinkwrap: requested, fakeChild: sw, fromBundle: sw.bundled ? tree.fromBundle || tree : null, path: childPath(tree.path, pkg), - realpath: requested.type === 'directory' ? requested.fetchSpec : childPath(tree.realpath, pkg), + realpath: isDirectory ? requested.fetchSpec : childPath(tree.realpath, pkg), location: (tree.location === '/' ? '' : tree.location + '/') + pkg.name, - isLink: requested.type === 'directory', + isLink: isDirectory, isInLink: tree.isLink || tree.isInLink, swRequires: sw.requires }) @@ -195,7 +203,7 @@ function fetchChild (topPath, tree, sw, requested) { var isLink = pkg._requested.type === 'directory' const child = createChild({ package: pkg, - loaded: false, + loaded: isLink, parent: tree, fromShrinkwrap: requested, path: childPath(tree.path, pkg), diff --git a/test/tap/install-from-local-multipath.js b/test/tap/install-from-local-multipath.js index 83dbdadde9e55..d2b3df4415346 100644 --- a/test/tap/install-from-local-multipath.js +++ b/test/tap/install-from-local-multipath.js @@ -167,9 +167,13 @@ test('\'npm install\' should install local packages', function (t) { 'install', '.' ], EXEC_OPTS, - function (err, code) { + function (err, code, stdout, stderr) { t.ifError(err, 'error should not exist') t.notOk(code, 'npm install exited with code 0') + // if the test fails, let's see why + if (err || code) { + console.error({code, stdout, stderr}) + } t.end() } ) diff --git a/test/tap/install-symlink-leave-children-alone.js b/test/tap/install-symlink-leave-children-alone.js new file mode 100644 index 0000000000000..cb7a4f3433775 --- /dev/null +++ b/test/tap/install-symlink-leave-children-alone.js @@ -0,0 +1,127 @@ +const common = require('../common-tap.js') +const Tacks = require('tacks') +const {File, Dir} = Tacks +const pkg = common.pkg +const t = require('tap') + +// via https://github.com/chhetrisushil/npm-update-test +const goodPackage2Entry = { + version: 'file:../package2', + dev: true +} + +const badPackage2Entry = { + version: 'file:../package2', + dev: true, + dependencies: { + '@test/package3': { + version: '1.0.0' + } + } +} + +const goodPackage1Deps = { + '@test/package2': goodPackage2Entry, + '@test/package3': { + version: 'file:../package3', + dev: true + } +} + +const badPackage1Deps = { + '@test/package2': badPackage2Entry, + '@test/package3': { + version: 'file:../package3', + dev: true + } +} + +const badPackage1Lock = { + name: 'package1', + version: '1.0.0', + lockfileVersion: 1, + requires: true, + dependencies: badPackage1Deps +} + +const goodPackage1Lock = { + name: 'package1', + version: '1.0.0', + lockfileVersion: 1, + requires: true, + dependencies: goodPackage1Deps +} + +const package2Lock = { + name: 'package2', + version: '1.0.0', + lockfileVersion: 1, + requires: true, + dependencies: { + '@test/package3': { + version: 'file:../package3', + dev: true + } + } +} + +const package3Lock = { + name: 'package3', + version: '1.0.0', + lockfileVersion: 1 +} + +t.test('setup fixture', t => { + const fixture = new Tacks(new Dir({ + package1: new Dir({ + 'package-lock.json': new File(badPackage1Lock), + 'package.json': new File({ + name: 'package1', + version: '1.0.0', + devDependencies: { + '@test/package2': 'file:../package2', + '@test/package3': 'file:../package3' + } + }) + }), + package2: new Dir({ + 'package-lock.json': new File(package2Lock), + 'package.json': new File({ + name: 'package2', + version: '1.0.0', + devDependencies: { + '@test/package3': 'file:../package3' + } + }) + }), + package3: new Dir({ + 'package-lock.json': new File(package3Lock), + 'package.json': new File({ + name: 'package3', + version: '1.0.0' + }) + }) + })) + fixture.create(pkg) + t.end() +}) + +t.test('install does not error', t => + common.npm(['install', '--no-registry'], { cwd: pkg + '/package1' }) + .then(([code, out, err]) => { + t.equal(code, 0) + console.error({out, err}) + })) + +t.test('updated package-lock.json to good state, left others alone', t => { + const fs = require('fs') + const getlock = n => { + const file = pkg + '/package' + n + '/package-lock.json' + const lockdata = fs.readFileSync(file, 'utf8') + return JSON.parse(lockdata) + } + t.strictSame(getlock(1), goodPackage1Lock) + t.strictSame(getlock(2), package2Lock) + t.strictSame(getlock(3), package3Lock) + t.end() +})