Skip to content

Commit

Permalink
bundles: Refactor the bundled module model (again)
Browse files Browse the repository at this point in the history
This rejiggers bundled modules, such that they're moved out of the bundling
module's folder at extract time and places in staging by themselves like any
other module. This allows the rest of npm to pretend they're normal modules,
which lets us remove a whole host of special cases and overall simplifies
our code.

Fixes: #10482

So if you had:
```
a bundles b requres c
```
And the version of bundled `c` in `b` doesn't match the `package.json` it
will end up installing a new copy of `c` under `b` and ditching the contents
of `b`.

PR-URL: npm/npm#10487
Credit: @iarna
Reviewed By: @othiym23
  • Loading branch information
iarna committed Nov 20, 2015
1 parent 7f28462 commit aed71fb
Show file tree
Hide file tree
Showing 7 changed files with 161 additions and 82 deletions.
72 changes: 65 additions & 7 deletions lib/install/action/extract.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,77 @@
'use strict'
var path = require('path')
var iferr = require('iferr')
var asyncMap = require('slide').asyncMap
var rename = require('graceful-fs').rename
var gentlyRm = require('../../utils/gently-rm.js')
var updatePackageJson = require('../update-package-json')
var npm = require('../../npm.js')
var moduleName = require('../../utils/module-name.js')
var packageId = require('../../utils/package-id.js')
var cache = require('../../cache.js')
var buildPath = require('../build-path.js')

module.exports = function (top, buildpath, pkg, log, next) {
log.silly('extract', packageId(pkg))
var up = npm.config.get('unsafe-perm')
var user = up ? null : npm.config.get('user')
var group = up ? null : npm.config.get('group')
cache.unpack(pkg.package.name, pkg.package.version
, buildpath
, null, null, user, group,
function (er) {
if (er) return next(er)
updatePackageJson(pkg, buildpath, next)
})
cache.unpack(pkg.package.name, pkg.package.version, buildpath, null, null, user, group,
andUpdatePackageJson(pkg, buildpath, andStageBundledChildren(pkg, buildpath, log, next)))
}

function andUpdatePackageJson (pkg, buildpath, next) {
return iferr(next, function () {
updatePackageJson(pkg, buildpath, next)
})
}

function andStageBundledChildren (pkg, buildpath, log, next) {
var staging = path.resolve(buildpath, '..')
return iferr(next, function () {
asyncMap(pkg.children, andStageBundledModule(pkg, staging, buildpath), cleanupBundled)
})
function cleanupBundled () {
gentlyRm(path.join(buildpath, 'node_modules'), next)
}
}

function andStageBundledModule (bundler, staging, parentPath) {
return function (child, next) {
stageBundledModule(bundler, child, staging, parentPath, next)
}
}

function getTree (pkg) {
while (pkg.parent) pkg = pkg.parent
return pkg
}

function warn (pkg, code, msg) {
var tree = getTree(pkg)
var err = new Error(msg)
err.code = code
tree.warnings.push(err)
}

function stageBundledModule (bundler, child, staging, parentPath, next) {
var stageFrom = path.join(parentPath, 'node_modules', child.package.name)
var stageTo = buildPath(staging, child)

asyncMap(child.children, andStageBundledModule(bundler, staging, stageFrom), iferr(next, moveModule))

function moveModule () {
if (child.fromBundle) {
return rename(stageFrom, stageTo, iferr(next, updateMovedPackageJson))
} else {
warn(bundler, 'EBUNDLEOVERRIDE', 'In ' + packageId(bundler) +
' replacing bundled version of ' + moduleName(child) +
' with ' + packageId(child))
return gentlyRm(stageFrom, next)
}
}

function updateMovedPackageJson () {
updatePackageJson(child, stageTo, next)
}
}
31 changes: 1 addition & 30 deletions lib/install/action/finalize.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,6 @@ var rimraf = require('rimraf')
var fs = require('graceful-fs')
var mkdirp = require('mkdirp')
var asyncMap = require('slide').asyncMap
var iferr = require('iferr')

function getTree (pkg) {
while (pkg.parent) pkg = pkg.parent
return pkg
}

function warn (pkg, code, msg) {
var tree = getTree(pkg)
var err = new Error(msg)
err.code = code
tree.warnings.push(err)
}

function pathToShortname (modpath) {
return modpath.replace(/node_modules[/]/g, '').replace(/[/]/g, ' > ')
}

module.exports = function (top, buildpath, pkg, log, next) {
log.silly('finalize', pkg.path)
Expand Down Expand Up @@ -75,21 +58,9 @@ module.exports = function (top, buildpath, pkg, log, next) {
function moveModules (mkdirEr, files) {
if (mkdirEr) return next(mkdirEr)
asyncMap(files, function (file, done) {
// `from` wins over `to`, because if `from` was there it's because the
// module installer wanted it to be there. By contrast, `to` is just
// whatever was bundled in this module. And the intentions of npm's
// installer should always beat out random module contents.
var from = path.join(delpath, 'node_modules', file)
var to = path.join(pkg.path, 'node_modules', file)
fs.stat(to, function (er, info) {
if (er) return fs.rename(from, to, done)

var shortname = pathToShortname(path.relative(getTree(pkg).path, to))
warn(pkg, 'EBUNDLEOVERRIDE', 'Replacing bundled ' + shortname + ' with new installed version')
rimraf(to, iferr(done, function () {
fs.rename(from, to, done)
}))
})
fs.rename(from, to, done)
}, cleanup)
}

Expand Down
4 changes: 2 additions & 2 deletions lib/install/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ var path = require('path')
var validate = require('aproba')
var chain = require('slide').chain
var asyncMap = require('slide').asyncMap
var uniqueFilename = require('unique-filename')
var log = require('npmlog')
var andFinishTracker = require('./and-finish-tracker.js')
var andAddParentToErrors = require('./and-add-parent-to-errors.js')
var failedDependency = require('./deps.js').failedDependency
var packageId = require('../utils/package-id.js')
var moduleName = require('../utils/module-name.js')
var buildPath = require('./build-path.js')

var actions = {}

Expand Down Expand Up @@ -83,8 +83,8 @@ function prepareAction (staging, log) {
var cmd = action[0]
var pkg = action[1]
if (!actions[cmd]) throw new Error('Unknown decomposed command "' + cmd + '" (is it new?)')
var buildpath = uniqueFilename(staging, moduleName(pkg), pkg.realpath)
var top = path.resolve(staging, '../..')
var buildpath = buildPath(staging, pkg)
return [actions[cmd], top, buildpath, pkg, log.newGroup(cmd + ':' + moduleName(pkg))]
}
}
Expand Down
8 changes: 8 additions & 0 deletions lib/install/build-path.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
'use strict'
var uniqueFilename = require('unique-filename')
var moduleName = require('../utils/module-name.js')

module.exports = buildPath
function buildPath (staging, pkg) {
return uniqueFilename(staging, moduleName(pkg), pkg.realpath)
}
30 changes: 12 additions & 18 deletions lib/install/decompose-actions.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict'
var validate = require('aproba')
var asyncMap = require('slide').asyncMap
var npm = require('../npm.js')

module.exports = function (differences, decomposed, next) {
validate('AAF', arguments)
Expand All @@ -15,9 +16,6 @@ module.exports = function (differences, decomposed, next) {
case 'move':
moveSteps(decomposed, pkg, done)
break
case 'rebuild':
rebuildSteps(decomposed, pkg, done)
break
case 'remove':
case 'update-linked':
default:
Expand All @@ -27,13 +25,17 @@ module.exports = function (differences, decomposed, next) {
}

function addSteps (decomposed, pkg, done) {
decomposed.push(['fetch', pkg])
decomposed.push(['extract', pkg])
decomposed.push(['preinstall', pkg])
decomposed.push(['build', pkg])
decomposed.push(['install', pkg])
decomposed.push(['postinstall', pkg])
decomposed.push(['test', pkg])
if (!pkg.fromBundle) {
decomposed.push(['fetch', pkg])
decomposed.push(['extract', pkg])
decomposed.push(['test', pkg])
}
if (!pkg.fromBundle || npm.config.get('rebuild-bundle')) {
decomposed.push(['preinstall', pkg])
decomposed.push(['build', pkg])
decomposed.push(['install', pkg])
decomposed.push(['postinstall', pkg])
}
decomposed.push(['finalize', pkg])
done()
}
Expand All @@ -47,14 +49,6 @@ function moveSteps (decomposed, pkg, done) {
done()
}

function rebuildSteps (decomposed, pkg, done) {
decomposed.push(['preinstall', pkg])
decomposed.push(['build', pkg])
decomposed.push(['install', pkg])
decomposed.push(['postinstall', pkg])
done()
}

function defaultSteps (decomposed, cmd, pkg, done) {
decomposed.push([cmd, pkg])
done()
Expand Down
6 changes: 1 addition & 5 deletions lib/install/diff-trees.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
'use strict'
var validate = require('aproba')
var npa = require('npm-package-arg')
var npm = require('../npm.js')
var flattenTree = require('./flatten-tree.js')

function nonRegistrySource (pkg) {
Expand Down Expand Up @@ -130,10 +129,7 @@ function diffTrees (oldTree, newTree) {
pkg.isInLink = (pkg.oldPkg && isLink(pkg.oldPkg.parent)) ||
(pkg.parent && isLink(pkg.parent)) ||
requiredByAllLinked(pkg)
if (pkg.fromBundle) {
if (npm.config.get('rebuild-bundle')) differences.push(['rebuild', pkg])
if (pkg.oldPkg) differences.push(['remove', pkg])
} else if (pkg.oldPkg) {
if (pkg.oldPkg) {
if (!pkg.userRequired && pkgAreEquiv(pkg.oldPkg.package, pkg.package)) return
if (!pkg.isInLink && (isLink(pkg.oldPkg) || isLink(pkg))) {
differences.push(['update-linked', pkg])
Expand Down
92 changes: 72 additions & 20 deletions test/tap/override-bundled.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,41 +6,73 @@ var rimraf = require('rimraf')
var path = require('path')
var common = require('../common-tap.js')

var testdir = path.resolve(__dirname, path.basename(__filename, '.js'))
var testjson = {
dependencies: {'top-test': 'file:top-test/'}
}

var testname = path.basename(__filename, '.js')
var testdir = path.resolve(__dirname, testname)
var testmod = path.resolve(testdir, 'top-test')
var testmodjson = {
name: 'top-test',
version: '1.0.0',
dependencies: {
'bundle-update': 'file:bundle-update/',
'bundle-keep': 'file:bundle-keep/'
},
bundledDependencies: ['bundle-update', 'bundle-keep']
}

var bundleupdatesrc = path.resolve(testmod, 'bundle-update')
var bundleupdateNEW = path.resolve(bundleupdatesrc, 'NEW')
var bundleupdateNEWpostinstall = path.resolve(testdir, 'node_modules', 'top-test', 'node_modules', 'bundle-update', 'NEW')
var bundleupdatebad = path.resolve(testmod, 'node_modules', 'bundle-update')

var bundlekeepsrc = path.resolve(testmod, 'bundle-keep')
var bundlekeep = path.resolve(testmod, 'node_modules', 'bundle-keep')
var bundlekeepOLD = path.resolve(bundlekeep, 'OLD')
var bundlekeepOLDpostinstall = path.resolve(testdir, 'node_modules', 'top-test', 'node_modules', 'bundle-keep', 'OLD')

var bundledeepsrc = path.resolve(testmod, 'bundle-deep')
var bundledeep = path.resolve(testmod, 'node_modules', 'bundle-deep')
var bundledeepOLD = path.resolve(bundledeep, 'OLD')
var bundledeepOLDpostinstall = path.resolve(testdir, 'node_modules', 'top-test', 'node_modules', 'bundle-deep', 'OLD')

var bundledeepupdatesrc = path.resolve(testmod, 'bundle-deep-update')
var bundledeepupdate = path.resolve(bundledeep, 'node_modules', 'bundle-deep-update')
var bundledeepupdateNEW = path.resolve(bundledeepupdatesrc, 'NEW')
var bundledeepupdateNEWpostinstall = path.resolve(testdir, 'node_modules', 'top-test',
'node_modules', 'bundle-deep', 'node_modules', 'bundle-deep-update', 'NEW')

var testjson = {
dependencies: {'top-test': 'file:top-test/'}
}

var testmodjson = {
name: 'top-test',
version: '1.0.0',
dependencies: {
'bundle-update': bundleupdatesrc,
'bundle-keep': bundlekeepsrc,
'bundle-deep': bundledeepsrc
},
bundledDependencies: ['bundle-update', 'bundle-keep', 'bundle-deep']
}
var bundlejson = {
name: 'bundle-update',
version: '1.0.0'
}

var bundlekeepjson = {
name: 'bundle-keep',
version: '1.0.0',
_requested: {
rawSpec: 'file:bundle-keep/'
rawSpec: bundlekeepsrc
}
}
var bundledeepjson = {
name: 'bundle-deep',
version: '1.0.0',
dependencies: {
'bundle-deep-update': bundledeepupdatesrc
},
_requested: {
rawSpec: bundledeepsrc
}
}

var bundledeepupdatejson = {
version: '1.0.0',
name: 'bundle-deep-update'
}

function writepjs (dir, content) {
fs.writeFileSync(path.join(dir, 'package.json'), JSON.stringify(content, null, 2))
}
Expand All @@ -50,16 +82,30 @@ function setup () {
writepjs(testdir, testjson)
mkdirp.sync(testmod)
writepjs(testmod, testmodjson)

mkdirp.sync(bundleupdatesrc)
writepjs(bundleupdatesrc, bundlejson)
fs.writeFileSync(bundleupdateNEW, '')
mkdirp.sync(bundleupdatebad)
writepjs(bundleupdatebad, bundlejson)

mkdirp.sync(bundlekeepsrc)
writepjs(bundlekeepsrc, bundlekeepjson)
mkdirp.sync(bundlekeep)
writepjs(bundlekeep, bundlekeepjson)
fs.writeFileSync(bundlekeepOLD, '')

mkdirp.sync(bundledeepsrc)
writepjs(bundledeepsrc, bundledeepjson)
mkdirp.sync(bundledeep)
writepjs(bundledeep, bundledeepjson)
fs.writeFileSync(bundledeepOLD, '')

mkdirp.sync(bundledeepupdatesrc)
writepjs(bundledeepupdatesrc, bundledeepupdatejson)
mkdirp.sync(bundledeepupdate)
writepjs(bundledeepupdate, bundledeepupdatejson)
fs.writeFileSync(bundledeepupdateNEW, '')
}

function cleanup () {
Expand All @@ -75,7 +121,7 @@ test('setup', function (t) {
test('bundled', function (t) {
common.npm(['install', '--loglevel=warn'], {cwd: testdir}, function (err, code, stdout, stderr) {
if (err) throw err
t.plan(5)
t.plan(8)
t.is(code, 0, 'npm itself completed ok')

// This tests that after the install we have a freshly installed version
Expand All @@ -86,16 +132,22 @@ test('bundled', function (t) {
// metadata) installed in that location and will go off and try to do
// _things_ to it. Things like chmod in particular, which in turn results
// in the dreaded ENOENT errors.
t.like(stderr, /EPACKAGEJSON override-bundled/, "didn't stomp on other warnings")
t.like(stderr, /EBUNDLEOVERRIDE/, 'included warning about bundled dep')
t.like(stderr, new RegExp('EPACKAGEJSON ' + testname), "didn't stomp on other warnings")
t.like(stderr, /EBUNDLEOVERRIDE.*bundle-update/, 'included update warning about bundled dep')
t.like(stderr, /EBUNDLEOVERRIDE.*bundle-deep-update/, 'included update warning about deeply bundled dep')
fs.stat(bundleupdateNEWpostinstall, function (missing) {
t.ok(!missing, 'package.json overrode bundle')
})

fs.stat(bundledeepupdateNEWpostinstall, function (missing) {
t.ok(!missing, 'deep package.json overrode bundle')
})
// Relatedly, when upgrading, if a bundled module is replacing an existing
// module we want to choose the bundled version, not the version we're replacing.
fs.stat(bundlekeepOLDpostinstall, function (missing) {
t.ok(!missing, 'package.json overrode bundle')
t.ok(!missing, 'no override when package.json matches')
})
fs.stat(bundledeepOLDpostinstall, function (missing) {
t.ok(!missing, 'deep no override when package.json matches')
})
})
})
Expand Down

0 comments on commit aed71fb

Please sign in to comment.