From 2fb0509f3c15dfd1b1013fd41650b12cc47e009f Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Thu, 18 Jul 2019 15:35:20 -0700 Subject: [PATCH] Fix `npm ci` with `file:` dependencies Partially reverts #40/#86, keeping the "Don't record linked deps as bundled" part but reverting the "Don't iterate into linked deps" part. It seems that we need to record dependencies of linked deps in order for `npm ci` to work. Fix: https://npm.community/t/6-8-0-npm-ci-fails-with-local-dependency/5385 Fix: https://npm.community/t/npm-ci-fail-to-local-packages/6076 PR-URL: https://github.com/npm/cli/pull/216 Credit: @jfirebaugh Close: #216 Reviewed-by: @isaacs EDIT: Updated test to not rely on network and follow latest and greatest test patterns. --- lib/shrinkwrap.js | 3 +- test/tap/ci-with-local-dependency.js | 82 ++++++++++++++++++++++++++++ test/tap/spec-local-specifiers.js | 4 +- 3 files changed, 86 insertions(+), 3 deletions(-) create mode 100644 test/tap/ci-with-local-dependency.js diff --git a/lib/shrinkwrap.js b/lib/shrinkwrap.js index 75d58bf8e4f0b..bd8c0abbaa205 100644 --- a/lib/shrinkwrap.js +++ b/lib/shrinkwrap.js @@ -111,7 +111,6 @@ function shrinkwrapDeps (deps, top, tree, seen) { var pkginfo = deps[moduleName(child)] = {} var requested = getRequested(child) || child.package._requested || {} var linked = child.isLink || child.isInLink - var linkedFromHere = linked && path.relative(top.realpath, child.realpath)[0] !== '.' pkginfo.version = childVersion(top, child, requested) if (requested.type === 'git' && child.package._from) { pkginfo.from = child.package._from @@ -142,7 +141,7 @@ function shrinkwrapDeps (deps, top, tree, seen) { }) } // iterate into children on non-links and links contained within the top level package - if (child.children.length && (!child.isLink || linkedFromHere)) { + if (child.children.length) { pkginfo.dependencies = {} shrinkwrapDeps(pkginfo.dependencies, top, child, seen) } diff --git a/test/tap/ci-with-local-dependency.js b/test/tap/ci-with-local-dependency.js new file mode 100644 index 0000000000000..376dc97818153 --- /dev/null +++ b/test/tap/ci-with-local-dependency.js @@ -0,0 +1,82 @@ +const fs = require('graceful-fs') +const path = require('path') + +const mkdirp = require('mkdirp') +const t = require('tap') + +const common = require('../common-tap.js') + +const pkg = common.pkg + '/package' + +const EXEC_OPTS = { + cwd: pkg, + stdio: [0, 1, 2], + env: common.newEnv().extend({ + npm_config_registry: common.registry + }) +} + +const localDependencyJson = { + name: 'local-dependency', + version: '0.0.0', + dependencies: { + 'test-package': '0.0.0' + } +} + +const dependentJson = { + name: 'dependent', + version: '0.0.0', + dependencies: { + 'local-dependency': '../local-dependency' + } +} + +const target = path.resolve(pkg, '../local-dependency') +const mr = require('npm-registry-mock') +let server +t.teardown(() => { + if (server) { + server.close() + } +}) + +t.test('setup', function (t) { + mkdirp.sync(target) + fs.writeFileSync( + path.join(target, 'package.json'), + JSON.stringify(localDependencyJson, null, 2) + ) + mkdirp.sync(pkg) + fs.writeFileSync( + path.join(pkg, 'package.json'), + JSON.stringify(dependentJson, null, 2) + ) + mr({ port: common.port }, (er, s) => { + if (er) { + throw er + } + server = s + t.end() + }) +}) + +t.test('\'npm install\' should install local pkg from sub path', function (t) { + common.npm(['install', '--loglevel=silent'], EXEC_OPTS, function (err, code) { + if (err) throw err + t.equal(code, 0, 'npm install exited with code') + t.ok(fs.statSync(path.resolve(pkg, 'node_modules/local-dependency/package.json')).isFile(), 'local dependency package.json exists') + t.ok(fs.statSync(path.resolve(pkg, 'node_modules/local-dependency/node_modules/test-package')).isDirectory(), 'transitive dependency installed') + t.end() + }) +}) + +t.test('\'npm ci\' should work', function (t) { + common.npm(['ci', '--loglevel=silent'], EXEC_OPTS, function (err, code) { + if (err) throw err + t.equal(code, 0, 'npm install exited with code') + t.ok(fs.statSync(path.resolve(pkg, 'node_modules/local-dependency/package.json')).isFile(), 'local dependency package.json exists') + t.ok(fs.statSync(path.resolve(pkg, 'node_modules/local-dependency/node_modules/test-package')).isDirectory(), 'transitive dependency installed') + t.end() + }) +}) diff --git a/test/tap/spec-local-specifiers.js b/test/tap/spec-local-specifiers.js index e1aa29cee4d3b..6ea65278cda26 100644 --- a/test/tap/spec-local-specifiers.js +++ b/test/tap/spec-local-specifiers.js @@ -585,7 +585,9 @@ test('save behavior', function (t) { var deps = pjson.dependencies || {} t.is(deps['sb-transitive'], 'file:../sb-transitive', 'package.json') var sdep = shrinkwrap.dependencies['sb-transitive'] || {} - t.like(sdep, {bundled: null, dependencies: null, version: 'file:../sb-transitive'}, 'npm-shrinkwrap.json direct dep') + var tdep = sdep.dependencies.sbta + t.like(tdep, {bundled: null, version: 'file:../sb-transitive/sbta'}, 'npm-shrinkwrap.json transitive dep') + t.like(sdep, {bundled: null, version: 'file:../sb-transitive'}, 'npm-shrinkwrap.json direct dep') t.done() }) })