From 38d91de97eb3b44dfce471af549a9474dcb6909e Mon Sep 17 00:00:00 2001 From: Martin Packman Date: Wed, 6 Nov 2019 19:43:59 +0000 Subject: [PATCH 1/3] Fix package id in shrinkwrap lifecycle step output Currently all logging related to shrinkwrap steps reports 'undefined' for the package in output and log messages. This is due to the package associated with the `idealTree` being recreated in the `savePackageJson()` method which precedes these steps. For now, just copy forward the `_id` attribute which lifecycle logging expects, but note that mutating `package` here is surprising. Fixes npm/npm#20756 --- lib/install/save.js | 5 +++++ test/common-tap.js | 5 +++++ test/tap/install-lifecycle.js | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+) create mode 100644 test/tap/install-lifecycle.js diff --git a/lib/install/save.js b/lib/install/save.js index 92b44a108099b..b4633204b60ff 100644 --- a/lib/install/save.js +++ b/lib/install/save.js @@ -63,6 +63,8 @@ function savePackageJson (tree, next) { fs.readFile(saveTarget, 'utf8', iferr(next, function (packagejson) { const indent = detectIndent(packagejson).indent const newline = detectNewline(packagejson) + // TODO - mutating tree.package here is surprising, do something else? + const currentPackage = tree.package try { tree.package = parseJSON(packagejson) } catch (ex) { @@ -131,6 +133,9 @@ function savePackageJson (tree, next) { } else { writeFileAtomic(saveTarget, json, next) } + + // Restore derived id from package before reloading and mutation. + tree.package._id = currentPackage._id })) } diff --git a/test/common-tap.js b/test/common-tap.js index 86a90571216cf..d0426ba6e1176 100644 --- a/test/common-tap.js +++ b/test/common-tap.js @@ -282,3 +282,8 @@ Environment.prototype.extend = function (env) { } return self } + +var reEscape = /[\\[\](){}*?+.^$|]/g +exports.escapeForRe = function (string) { + return string.replace(reEscape, '\\$&') +} diff --git a/test/tap/install-lifecycle.js b/test/tap/install-lifecycle.js new file mode 100644 index 0000000000000..a6fc46b9197ee --- /dev/null +++ b/test/tap/install-lifecycle.js @@ -0,0 +1,35 @@ +var fs = require('graceful-fs') +var path = require('path') +var test = require('tap').test +var common = require('../common-tap.js') +var pkg = common.pkg + +test('npm install execution order', function (t) { + const packageJson = { + name: 'life-test', + version: '0.0.1', + description: 'Test for npm install execution order', + scripts: { + install: 'true', + preinstall: 'true', + preshrinkwrap: 'true', + postinstall: 'true', + postshrinkwrap: 'true', + shrinkwrap: 'true' + } + } + fs.writeFileSync(path.resolve(pkg, 'package.json'), JSON.stringify(packageJson), 'utf8') + common.npm(['install', '--loglevel=error'], { cwd: pkg }, function (err, code, stdout, stderr) { + if (err) throw err + + t.comment(stdout) + t.comment(stderr) + + const steps = ['preinstall', 'install', 'postinstall', 'preshrinkwrap', 'shrinkwrap', 'postshrinkwrap'] + const expectedLines = steps.map(function (step) { + return '> ' + packageJson.name + '@' + packageJson.version + ' ' + step + }) + t.match(stdout, new RegExp(expectedLines.map(common.escapeForRe).join('.*'), 's')) + t.end() + }) +}) From b9ebb12af66562a707e69e8c312bc32876af701a Mon Sep 17 00:00:00 2001 From: Martin Packman Date: Thu, 7 Nov 2019 14:44:46 +0000 Subject: [PATCH 2/3] Avoid the RegExp 's' flag Not supported on node 6 apparently. Can get the same effect by explictly matching the newline character. --- test/tap/install-lifecycle.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/tap/install-lifecycle.js b/test/tap/install-lifecycle.js index a6fc46b9197ee..c5b0fd35a020e 100644 --- a/test/tap/install-lifecycle.js +++ b/test/tap/install-lifecycle.js @@ -29,7 +29,7 @@ test('npm install execution order', function (t) { const expectedLines = steps.map(function (step) { return '> ' + packageJson.name + '@' + packageJson.version + ' ' + step }) - t.match(stdout, new RegExp(expectedLines.map(common.escapeForRe).join('.*'), 's')) + t.match(stdout, new RegExp(expectedLines.map(common.escapeForRe).join('(.|\n)*'))) t.end() }) }) From ef69276aa95d7ec3624aa3113ddb84afd683ea72 Mon Sep 17 00:00:00 2001 From: Martin Date: Sat, 23 May 2020 14:17:27 +0100 Subject: [PATCH 3/3] Tweak code comments and method of restoring id --- lib/install.js | 3 +++ lib/install/save.js | 7 +++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/lib/install.js b/lib/install.js index a4cf2b186de51..3254760fef03a 100644 --- a/lib/install.js +++ b/lib/install.js @@ -649,6 +649,9 @@ Installer.prototype.saveToDependencies = function (cb) { validate('F', arguments) if (this.failing) return cb() log.silly('install', 'saveToDependencies') + // Note idealTree will be mutated during the save operations below as the + // package is reloaded from disk to preserve additional details. This means + // steps after postInstall will see a slightly different package object. if (this.saveOnlyLock) { saveShrinkwrap(this.idealTree, cb) } else { diff --git a/lib/install/save.js b/lib/install/save.js index b4633204b60ff..986233a516a7c 100644 --- a/lib/install/save.js +++ b/lib/install/save.js @@ -8,6 +8,7 @@ const iferr = require('iferr') const log = require('npmlog') const moduleName = require('../utils/module-name.js') const npm = require('../npm.js') +const packageId = require('../utils/package-id.js') const parseJSON = require('../utils/parse-json.js') const path = require('path') const stringifyPackage = require('stringify-package') @@ -63,8 +64,6 @@ function savePackageJson (tree, next) { fs.readFile(saveTarget, 'utf8', iferr(next, function (packagejson) { const indent = detectIndent(packagejson).indent const newline = detectNewline(packagejson) - // TODO - mutating tree.package here is surprising, do something else? - const currentPackage = tree.package try { tree.package = parseJSON(packagejson) } catch (ex) { @@ -134,8 +133,8 @@ function savePackageJson (tree, next) { writeFileAtomic(saveTarget, json, next) } - // Restore derived id from package before reloading and mutation. - tree.package._id = currentPackage._id + // Restore derived id as it was removed when reloading from disk + tree.package._id = packageId(tree.package) })) }