Skip to content

Commit

Permalink
Preserve builtin conf when installing npm globally
Browse files Browse the repository at this point in the history
When a file named 'npmrc' is in the root of the npm module that is
currently running, it adds config values that override the defaults (but
not the global or user configs).

This is a way for a system package installer to tell the npm that it
installs to put its globals somewhere other than the default.  In order
to keep these configs around when users self-update npm with `npm i -g
npm`, these config values must be "sticky", and ride along into the
newly globally installed npm.

This commit restores this behavior, fixing self-updating npm for Windows
users, and any other systems that may make use of this functionality.

Fixes: #2002

PR-URL: #2184
Credit: @isaacs
Close: #2184
Reviewed-by: @ruyadorno
  • Loading branch information
isaacs authored and ruyadorno committed Nov 17, 2020
1 parent 4e522fd commit bc9afb1
Show file tree
Hide file tree
Showing 12 changed files with 173 additions and 52 deletions.
4 changes: 2 additions & 2 deletions lib/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ const Arborist = require('@npmcli/arborist')
const auditReport = require('npm-audit-report')
const npm = require('./npm.js')
const output = require('./utils/output.js')
const reifyOutput = require('./utils/reify-output.js')
const reifyFinish = require('./utils/reify-finish.js')
const auditError = require('./utils/audit-error.js')

const audit = async args => {
Expand All @@ -14,7 +14,7 @@ const audit = async args => {
const fix = args[0] === 'fix'
await arb.audit({ fix })
if (fix)
reifyOutput(arb)
await reifyFinish(arb)
else {
// will throw if there's an error, because this is an audit command
auditError(arb.auditReport)
Expand Down
4 changes: 2 additions & 2 deletions lib/ci.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const util = require('util')
const Arborist = require('@npmcli/arborist')
const rimraf = util.promisify(require('rimraf'))
const reifyOutput = require('./utils/reify-output.js')
const reifyFinish = require('./utils/reify-finish.js')

const log = require('npmlog')
const npm = require('./npm.js')
Expand Down Expand Up @@ -35,7 +35,7 @@ const ci = async () => {
])
// npm ci should never modify the lockfile or package.json
await arb.reify({ ...npm.flatOptions, save: false })
reifyOutput(arb)
await reifyFinish(arb)
}

module.exports = Object.assign(cmd, { completion, usage })
4 changes: 2 additions & 2 deletions lib/dedupe.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
const npm = require('./npm.js')
const Arborist = require('@npmcli/arborist')
const usageUtil = require('./utils/usage.js')
const reifyOutput = require('./utils/reify-output.js')
const reifyFinish = require('./utils/reify-finish.js')

const usage = usageUtil('dedupe', 'npm dedupe')
const completion = require('./utils/completion/none.js')
Expand All @@ -18,7 +18,7 @@ const dedupe = async (args) => {
dryRun,
})
await arb.dedupe(npm.flatOptions)
reifyOutput(arb)
await reifyFinish(arb)
}

module.exports = Object.assign(cmd, { usage, completion })
63 changes: 30 additions & 33 deletions lib/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@ const util = require('util')
const readdir = util.promisify(fs.readdir)
const npm = require('./npm.js')
const usageUtil = require('./utils/usage.js')
const reifyOutput = require('./utils/reify-output.js')
const reifyFinish = require('./utils/reify-finish.js')
const log = require('npmlog')
const { resolve, join } = require('path')
const Arborist = require('@npmcli/arborist')
const runScript = require('@npmcli/run-script')

const install = async (args, cb) => {
const cmd = async (args, cb) => install(args).then(() => cb()).catch(cb)

const install = async args => {
// the /path/to/node_modules/..
const globalTop = resolve(npm.globalDir, '..')
const { ignoreScripts, global: isGlobalInstall } = npm.flatOptions
Expand All @@ -34,38 +36,33 @@ const install = async (args, cb) => {
path: where,
})

try {
await arb.reify({
...npm.flatOptions,
add: args,
})
if (!args.length && !isGlobalInstall && !ignoreScripts) {
const { scriptShell } = npm.flatOptions
const scripts = [
'preinstall',
'install',
'postinstall',
'prepublish', // XXX should we remove this finally??
'preprepare',
'prepare',
'postprepare',
]
for (const event of scripts) {
await runScript({
path: where,
args: [],
scriptShell,
stdio: 'inherit',
stdioString: true,
event,
})
}
await arb.reify({
...npm.flatOptions,
add: args,
})
if (!args.length && !isGlobalInstall && !ignoreScripts) {
const { scriptShell } = npm.flatOptions
const scripts = [
'preinstall',
'install',
'postinstall',
'prepublish', // XXX should we remove this finally??
'preprepare',
'prepare',
'postprepare',
]
for (const event of scripts) {
await runScript({
path: where,
args: [],
scriptShell,
stdio: 'inherit',
stdioString: true,
event,
})
}
reifyOutput(arb)
cb()
} catch (er) {
cb(er)
}
await reifyFinish(arb)
}

const usage = usageUtil(
Expand Down Expand Up @@ -144,4 +141,4 @@ const completion = async (opts, cb) => {
cb()
}

module.exports = Object.assign(install, { usage, completion })
module.exports = Object.assign(cmd, { usage, completion })
6 changes: 3 additions & 3 deletions lib/link.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const semver = require('semver')

const npm = require('./npm.js')
const usageUtil = require('./utils/usage.js')
const reifyOutput = require('./utils/reify-output.js')
const reifyFinish = require('./utils/reify-finish.js')

const completion = (opts, cb) => {
const dir = npm.globalDir
Expand Down Expand Up @@ -122,7 +122,7 @@ const linkInstall = async args => {
add: names.map(l => `file:${resolve(globalTop, 'node_modules', l)}`),
})

reifyOutput(localArb)
await reifyFinish(localArb)
}

const linkPkg = async () => {
Expand All @@ -133,7 +133,7 @@ const linkPkg = async () => {
global: true,
})
await arb.reify({ add: [`file:${npm.prefix}`] })
reifyOutput(arb)
await reifyFinish(arb)
}

module.exports = Object.assign(cmd, { completion, usage })
4 changes: 2 additions & 2 deletions lib/prune.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const npm = require('./npm.js')
const Arborist = require('@npmcli/arborist')
const usageUtil = require('./utils/usage.js')

const reifyOutput = require('./utils/reify-output.js')
const reifyFinish = require('./utils/reify-finish.js')

const usage = usageUtil('prune',
'npm prune [[<@scope>/]<pkg>...] [--production]'
Expand All @@ -19,7 +19,7 @@ const prune = async () => {
path: where,
})
await arb.prune(npm.flatOptions)
reifyOutput(arb)
await reifyFinish(arb)
}

module.exports = Object.assign(cmd, { usage, completion })
4 changes: 2 additions & 2 deletions lib/uninstall.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const npm = require('./npm.js')
const rpj = require('read-package-json-fast')
const { resolve } = require('path')
const usageUtil = require('./utils/usage.js')
const reifyOutput = require('./utils/reify-output.js')
const reifyFinish = require('./utils/reify-finish.js')

const cmd = (args, cb) => rm(args).then(() => cb()).catch(cb)

Expand All @@ -32,7 +32,7 @@ const rm = async args => {
...npm.flatOptions,
rm: args,
})
reifyOutput(arb)
await reifyFinish(arb)
}

const usage = usageUtil(
Expand Down
4 changes: 2 additions & 2 deletions lib/update.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ const Arborist = require('@npmcli/arborist')
const log = require('npmlog')
const npm = require('./npm.js')
const usageUtil = require('./utils/usage.js')
const reifyOutput = require('./utils/reify-output.js')
const reifyFinish = require('./utils/reify-finish.js')
const completion = require('./utils/completion/installed-deep.js')

const usage = usageUtil(
Expand Down Expand Up @@ -32,7 +32,7 @@ const update = async args => {
})

await arb.reify({ update })
reifyOutput(arb)
await reifyFinish(arb)
}

module.exports = Object.assign(cmd, { usage, completion })
31 changes: 31 additions & 0 deletions lib/utils/reify-finish.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
const reifyOutput = require('./reify-output.js')
const npm = require('../npm.js')
const ini = require('ini')
const {writeFile} = require('fs').promises
const {resolve} = require('path')

const reifyFinish = async arb => {
await saveBuiltinConfig(arb)
reifyOutput(arb)
}

const saveBuiltinConfig = async arb => {
const { options: { global }, actualTree } = arb
if (!global)
return

// if we are using a builtin config, and just installed npm as
// a top-level global package, we have to preserve that config.
const npmNode = actualTree.inventory.get('node_modules/npm')
if (!npmNode)
return

const builtinConf = npm.config.data.get('builtin')
if (builtinConf.loadError)
return

const content = ini.stringify(builtinConf.raw).trim() + '\n'
await writeFile(resolve(npmNode.path, 'npmrc'), content)
}

module.exports = reifyFinish
15 changes: 15 additions & 0 deletions tap-snapshots/test-lib-utils-reify-finish.js-TAP.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/* IMPORTANT
* This snapshot file is auto-generated, but designed for humans.
* It should be checked into source control and tracked carefully.
* Re-generate by setting TAP_SNAPSHOT=1 and running tests.
* Make sure to inspect the output below. Do not ignore changes!
*/
'use strict'
exports[`test/lib/utils/reify-finish.js TAP should write if everything above passes > written config 1`] = `
hasBuiltinConfig=true
x=y
[nested]
foo=bar
`
8 changes: 4 additions & 4 deletions test/lib/audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const audit = require('../../lib/audit.js')
t.test('should audit using Arborist', t => {
let ARB_ARGS = null
let AUDIT_CALLED = false
let REIFY_OUTPUT_CALLED = false
let REIFY_FINISH_CALLED = false
let AUDIT_REPORT_CALLED = false
let OUTPUT_CALLED = false
let ARB_OBJ = null
Expand All @@ -32,11 +32,11 @@ t.test('should audit using Arborist', t => {
this.auditReport = {}
}
},
'../../lib/utils/reify-output.js': arb => {
'../../lib/utils/reify-finish.js': arb => {
if (arb !== ARB_OBJ) {
throw new Error('got wrong object passed to reify-output')
}
REIFY_OUTPUT_CALLED = true
REIFY_FINISH_CALLED = true
},
'../../lib/utils/output.js': () => {
OUTPUT_CALLED = true
Expand All @@ -55,7 +55,7 @@ t.test('should audit using Arborist', t => {

t.test('audit fix', t => {
audit(['fix'], () => {
t.equal(REIFY_OUTPUT_CALLED, true, 'called reify output')
t.equal(REIFY_FINISH_CALLED, true, 'called reify output')
t.end()
})
})
Expand Down
78 changes: 78 additions & 0 deletions test/lib/utils/reify-finish.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
const t = require('tap')
const requireInject = require('require-inject')

const npm = {
config: {
data: {
get: () => builtinConfMock,
},
},
}

const builtinConfMock = {
loadError: new Error('no builtin config'),
raw: { hasBuiltinConfig: true, x: 'y', nested: { foo: 'bar' }},
}

const reifyOutput = () => {}

let expectWrite = false
const realFs = require('fs')
const fs = {
...realFs,
promises: {
...realFs.promises,
writeFile: async (path, data) => {
if (!expectWrite)
throw new Error('did not expect to write builtin config file')
return realFs.promises.writeFile(path, data)
},
},
}

const reifyFinish = requireInject('../../../lib/utils/reify-finish.js', {
fs,
'../../../lib/npm.js': npm,
'../../../lib/utils/reify-output.js': reifyOutput,
})

t.test('should not write if not global', async t => {
expectWrite = false
await reifyFinish({
options: { global: false },
actualTree: {},
})
})

t.test('should not write if no global npm module', async t => {
expectWrite = false
await reifyFinish({
options: { global: true },
actualTree: {
inventory: new Map(),
},
})
})

t.test('should not write if builtin conf had load error', async t => {
expectWrite = false
await reifyFinish({
options: { global: true },
actualTree: {
inventory: new Map([['node_modules/npm', {}]]),
},
})
})

t.test('should write if everything above passes', async t => {
expectWrite = true
delete builtinConfMock.loadError
const path = t.testdir()
await reifyFinish({
options: { global: true },
actualTree: {
inventory: new Map([['node_modules/npm', {path}]]),
},
})
t.matchSnapshot(fs.readFileSync(`${path}/npmrc`, 'utf8'), 'written config')
})

0 comments on commit bc9afb1

Please sign in to comment.