From 1fafb51513466cd793866b576dfea9a8963a3335 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 28 Aug 2019 16:07:30 -0700 Subject: [PATCH] Revert "install: do not descend into directory deps' child modules" This reverts commit 45772af0ddca54b658cb2ba2182eec26d0a4729d Fix: https://npm.community/t/6-11-1-some-dependencies-are-no-longer-being-installed/9586/4 Also adds 2 tests to verify regression behavior. PR-URL: https://github.com/npm/cli/pull/242 Credit: @isaacs Close: #242 Reviewed-by: @claudiahdz --- lib/install/inflate-shrinkwrap.js | 20 +-- test/tap/install-from-local-multipath.js | 6 +- test/tap/install-link-metadeps-locally.js | 52 +++++++ test/tap/install-link-metadeps-subfolders.js | 68 ++++++++++ .../install-symlink-leave-children-alone.js | 127 ------------------ 5 files changed, 127 insertions(+), 146 deletions(-) create mode 100644 test/tap/install-link-metadeps-locally.js create mode 100644 test/tap/install-link-metadeps-subfolders.js delete 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 9fb38167b84a..b0b71ef6b132 100644 --- a/lib/install/inflate-shrinkwrap.js +++ b/lib/install/inflate-shrinkwrap.js @@ -50,12 +50,8 @@ 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) => { @@ -145,10 +141,6 @@ 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, @@ -164,7 +156,7 @@ function makeFakeChild (name, topPath, tree, sw, requested) { _spec: requested.rawSpec, _where: topPath, _args: [[requested.toString(), topPath]], - dependencies: isDirectory ? {} : sw.requires + dependencies: sw.requires } if (!sw.bundled) { @@ -175,16 +167,16 @@ function makeFakeChild (name, topPath, tree, sw, requested) { } const child = createChild({ package: pkg, - loaded: isDirectory, + loaded: true, parent: tree, children: [], fromShrinkwrap: requested, fakeChild: sw, fromBundle: sw.bundled ? tree.fromBundle || tree : null, path: childPath(tree.path, pkg), - realpath: isDirectory ? requested.fetchSpec : childPath(tree.realpath, pkg), + realpath: requested.type === 'directory' ? requested.fetchSpec : childPath(tree.realpath, pkg), location: (tree.location === '/' ? '' : tree.location + '/') + pkg.name, - isLink: isDirectory, + isLink: requested.type === 'directory', isInLink: tree.isLink || tree.isInLink, swRequires: sw.requires }) @@ -203,7 +195,7 @@ function fetchChild (topPath, tree, sw, requested) { var isLink = pkg._requested.type === 'directory' const child = createChild({ package: pkg, - loaded: isLink, + loaded: false, 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 d2b3df441534..83dbdadde9e5 100644 --- a/test/tap/install-from-local-multipath.js +++ b/test/tap/install-from-local-multipath.js @@ -167,13 +167,9 @@ test('\'npm install\' should install local packages', function (t) { 'install', '.' ], EXEC_OPTS, - function (err, code, stdout, stderr) { + function (err, code) { 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-link-metadeps-locally.js b/test/tap/install-link-metadeps-locally.js new file mode 100644 index 000000000000..136fd46d10bb --- /dev/null +++ b/test/tap/install-link-metadeps-locally.js @@ -0,0 +1,52 @@ +// XXX Remove in npm v7, when this is no longer how we do things +const t = require('tap') +const common = require('../common-tap.js') +const pkg = common.pkg +const mkdirp = require('mkdirp') +const { writeFileSync, statSync } = require('fs') +const { resolve } = require('path') +const mr = require('npm-registry-mock') +const rimraf = require('rimraf') + +t.test('setup', t => { + mkdirp.sync(resolve(pkg, 'node_modules')) + mkdirp.sync(resolve(pkg, 'foo')) + writeFileSync(resolve(pkg, 'foo', 'package.json'), JSON.stringify({ + name: 'foo', + version: '1.2.3', + dependencies: { + underscore: '*' + } + })) + + writeFileSync(resolve(pkg, 'package.json'), JSON.stringify({ + name: 'root', + version: '1.2.3', + dependencies: { + foo: 'file:foo' + } + })) + + mr({ port: common.port }, (er, s) => { + if (er) { + throw er + } + t.parent.teardown(() => s.close()) + t.end() + }) +}) + +t.test('initial install to create package-lock', + t => common.npm(['install', '--registry', common.registry], { cwd: pkg }) + .then(([code]) => t.equal(code, 0, 'command worked'))) + +t.test('remove node_modules', t => + rimraf(resolve(pkg, 'node_modules'), t.end)) + +t.test('install again from package-lock', t => + common.npm(['install', '--registry', common.registry], { cwd: pkg }) + .then(([code]) => { + t.equal(code, 0, 'command worked') + const underscore = resolve(pkg, 'node_modules', 'underscore') + t.equal(statSync(underscore).isDirectory(), true, 'underscore installed') + })) diff --git a/test/tap/install-link-metadeps-subfolders.js b/test/tap/install-link-metadeps-subfolders.js new file mode 100644 index 000000000000..7544c8a4ebe8 --- /dev/null +++ b/test/tap/install-link-metadeps-subfolders.js @@ -0,0 +1,68 @@ +const t = require('tap') +const common = require('../common-tap.js') +const mkdirp = require('mkdirp') +const { writeFileSync, readFileSync } = require('fs') +const { resolve } = require('path') +const pkg = common.pkg +const app = resolve(pkg, 'app') +const lib = resolve(pkg, 'lib') +const moda = resolve(lib, 'module-a') +const modb = resolve(lib, 'module-b') + +const rimraf = require('rimraf') + +t.test('setup', t => { + mkdirp.sync(app) + mkdirp.sync(moda) + mkdirp.sync(modb) + + writeFileSync(resolve(app, 'package.json'), JSON.stringify({ + name: 'app', + version: '1.2.3', + dependencies: { + moda: 'file:../lib/module-a' + } + })) + + writeFileSync(resolve(moda, 'package.json'), JSON.stringify({ + name: 'moda', + version: '1.2.3', + dependencies: { + modb: 'file:../module-b' + } + })) + + writeFileSync(resolve(modb, 'package.json'), JSON.stringify({ + name: 'modb', + version: '1.2.3' + })) + + t.end() +}) + +t.test('initial install to create package-lock', + t => common.npm(['install'], { cwd: app }) + .then(([code]) => t.equal(code, 0, 'command worked'))) + +t.test('remove node_modules', t => + rimraf(resolve(pkg, 'node_modules'), t.end)) + +t.test('install again from package-lock', t => + common.npm(['install'], { cwd: app }) + .then(([code]) => { + t.equal(code, 0, 'command worked') + // verify that module-b is linked under module-a + const depPkg = resolve( + app, + 'node_modules', + 'moda', + 'node_modules', + 'modb', + 'package.json' + ) + const data = JSON.parse(readFileSync(depPkg, 'utf8')) + t.strictSame(data, { + name: 'modb', + version: '1.2.3' + }) + })) diff --git a/test/tap/install-symlink-leave-children-alone.js b/test/tap/install-symlink-leave-children-alone.js deleted file mode 100644 index cb7a4f343377..000000000000 --- a/test/tap/install-symlink-leave-children-alone.js +++ /dev/null @@ -1,127 +0,0 @@ -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() -})