Skip to content

Commit

Permalink
install: do not descend into directory deps' child modules
Browse files Browse the repository at this point in the history
This builds on the work of #217, bringing
back the logic in 2f0c883 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: #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
  • Loading branch information
isaacs committed Aug 6, 2019
1 parent 24acc9f commit 45772af
Show file tree
Hide file tree
Showing 3 changed files with 146 additions and 7 deletions.
20 changes: 14 additions & 6 deletions lib/install/inflate-shrinkwrap.js
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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,
Expand All @@ -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) {
Expand All @@ -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
})
Expand All @@ -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),
Expand Down
6 changes: 5 additions & 1 deletion test/tap/install-from-local-multipath.js
Expand Up @@ -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()
}
)
Expand Down
127 changes: 127 additions & 0 deletions 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()
})

0 comments on commit 45772af

Please sign in to comment.