Skip to content

Commit

Permalink
Revert "install: do not descend into directory deps' child modules"
Browse files Browse the repository at this point in the history
This reverts commit 45772af

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: #242
Credit: @isaacs
Close: #242
Reviewed-by: @claudiahdz
  • Loading branch information
isaacs authored and claudiahdz committed Aug 30, 2019
1 parent 235ed1d commit 1fafb51
Show file tree
Hide file tree
Showing 5 changed files with 127 additions and 146 deletions.
20 changes: 6 additions & 14 deletions lib/install/inflate-shrinkwrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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,
Expand All @@ -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) {
Expand All @@ -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
})
Expand All @@ -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),
Expand Down
6 changes: 1 addition & 5 deletions test/tap/install-from-local-multipath.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
)
Expand Down
52 changes: 52 additions & 0 deletions test/tap/install-link-metadeps-locally.js
Original file line number Diff line number Diff line change
@@ -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')
}))
68 changes: 68 additions & 0 deletions test/tap/install-link-metadeps-subfolders.js
Original file line number Diff line number Diff line change
@@ -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'
})
}))
127 changes: 0 additions & 127 deletions test/tap/install-symlink-leave-children-alone.js

This file was deleted.

0 comments on commit 1fafb51

Please sign in to comment.