Skip to content

Commit

Permalink
fix: clean up npm object (#7416)
Browse files Browse the repository at this point in the history
Removed all the setters, they were dangerous and not the right way to
set those values.

Tweaked `this.#init` so it always returns an object so the coerce in
`this.load` didn't need to happen.
  • Loading branch information
wraithgar authored Apr 25, 2024
1 parent c060e60 commit 0e74ee4
Show file tree
Hide file tree
Showing 4 changed files with 17 additions and 57 deletions.
40 changes: 11 additions & 29 deletions lib/npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ class Npm {

async load () {
return time.start('npm:load', async () => {
const { exec = true } = await this.#load().then(r => r ?? {})
const { exec } = await this.#load()
return {
exec,
command: this.argv.shift(),
Expand Down Expand Up @@ -130,11 +130,12 @@ class Npm {
output.error('')
}

const logsMax = this.config.get('logs-max')

if (this.logFiles.length) {
log.error('', `A complete log of this run can be found in: ${this.logFiles}`)
} else if (logsMax <= 0) {
return log.error('', `A complete log of this run can be found in: ${this.logFiles}`)
}

const logsMax = this.config.get('logs-max')
if (logsMax <= 0) {
// user specified no log file
log.error('', `Log files were not written due to the config logs-max=${logsMax}`)
} else {
Expand All @@ -151,11 +152,6 @@ class Npm {
return this.#title
}

set title (t) {
process.title = t
this.#title = t
}

async #load () {
await time.start('npm:load:whichnode', async () => {
// TODO should we throw here?
Expand Down Expand Up @@ -210,8 +206,9 @@ class Npm {
const { parsedArgv: { cooked, remain } } = this.config
this.argv = remain
// Secrets are mostly in configs, so title is set using only the positional args
// to keep those from being leaked.
this.title = ['npm'].concat(replaceInfo(remain)).join(' ').trim()
// to keep those from being leaked. We still do a best effort replaceInfo.
this.#title = ['npm'].concat(replaceInfo(remain)).join(' ').trim()
process.title = this.#title
// The cooked argv is also logged separately for debugging purposes. It is
// cleaned as a best effort by replacing known secrets like basic auth
// password and strings that look like npm tokens. XXX: for this to be
Expand Down Expand Up @@ -252,6 +249,8 @@ class Npm {
this.argv = ['version']
this.config.set('usage', false, 'cli')
}

return { exec: true }
}

get isShellout () {
Expand Down Expand Up @@ -331,26 +330,14 @@ class Npm {
return this.config.get('cache')
}

set cache (r) {
this.config.set('cache', r)
}

get globalPrefix () {
return this.config.globalPrefix
}

set globalPrefix (r) {
this.config.globalPrefix = r
}

get localPrefix () {
return this.config.localPrefix
}

set localPrefix (r) {
this.config.localPrefix = r
}

get localPackage () {
return this.config.localPackage
}
Expand Down Expand Up @@ -386,11 +373,6 @@ class Npm {
return this.global ? this.globalPrefix : this.localPrefix
}

set prefix (r) {
const k = this.global ? 'globalPrefix' : 'localPrefix'
this[k] = r
}

get usage () {
return usage(this)
}
Expand Down
3 changes: 2 additions & 1 deletion test/lib/arborist-cmd.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ t.test('arborist-cmd', async t => {
chdir: (dirs) => dirs.testdir,
})

npm.localPrefix = prefix
// TODO there has to be a better way to do this
npm.config.localPrefix = prefix
await cmd.execWorkspaces([])

t.same(cmd.workspaceNames, ['a', 'c'], 'should set array with single ws name')
Expand Down
2 changes: 1 addition & 1 deletion test/lib/commands/exec.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ t.test('--prefix', async t => {
})

// This is what `--prefix` does
npm.globalPrefix = npm.localPrefix
npm.config.globalPrefix = npm.config.localPrefix

await registry.package({
manifest,
Expand Down
29 changes: 3 additions & 26 deletions test/lib/npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,8 @@ t.test('npm.load', async t => {
})

await t.test('basic loading', async t => {
const { npm, logs, prefix: dir, cache, other } = await loadMockNpm(t, {
const { npm, logs, cache } = await loadMockNpm(t, {
prefixDir: { node_modules: {} },
otherDirs: {
newCache: {},
},
config: {
timing: true,
},
Expand All @@ -61,25 +58,11 @@ t.test('npm.load', async t => {

mockGlobals(t, { process: { platform: 'posix' } })
t.equal(resolve(npm.cache), resolve(cache), 'cache is cache')
npm.cache = other.newCache
t.equal(npm.config.get('cache'), other.newCache, 'cache setter sets config')
t.equal(npm.cache, other.newCache, 'cache getter gets new config')
t.equal(npm.lockfileVersion, 2, 'lockfileVersion getter')
t.equal(npm.prefix, npm.localPrefix, 'prefix is local prefix')
t.not(npm.prefix, npm.globalPrefix, 'prefix is not global prefix')
npm.globalPrefix = npm.prefix
t.equal(npm.prefix, npm.globalPrefix, 'globalPrefix setter')
npm.localPrefix = dir + '/extra/prefix'
t.equal(npm.prefix, npm.localPrefix, 'prefix is local prefix after localPrefix setter')
t.not(npm.prefix, npm.globalPrefix, 'prefix is not global prefix after localPrefix setter')

npm.prefix = dir + '/some/prefix'
t.equal(npm.prefix, npm.localPrefix, 'prefix is local prefix after prefix setter')
t.not(npm.prefix, npm.globalPrefix, 'prefix is not global prefix after prefix setter')
t.equal(npm.bin, npm.localBin, 'bin is local bin after prefix setter')
t.not(npm.bin, npm.globalBin, 'bin is not global bin after prefix setter')
t.equal(npm.dir, npm.localDir, 'dir is local dir after prefix setter')
t.not(npm.dir, npm.globalDir, 'dir is not global dir after prefix setter')
t.equal(npm.bin, npm.localBin, 'bin is local bin')
t.not(npm.bin, npm.globalBin, 'bin is not global bin')

npm.config.set('global', true)
t.equal(npm.prefix, npm.globalPrefix, 'prefix is global prefix after setting global')
Expand All @@ -89,12 +72,6 @@ t.test('npm.load', async t => {
t.equal(npm.dir, npm.globalDir, 'dir is global dir after setting global')
t.not(npm.dir, npm.localDir, 'dir is not local dir after setting global')

npm.prefix = dir + '/new/global/prefix'
t.equal(npm.prefix, npm.globalPrefix, 'prefix is global prefix after prefix setter')
t.not(npm.prefix, npm.localPrefix, 'prefix is not local prefix after prefix setter')
t.equal(npm.bin, npm.globalBin, 'bin is global bin after prefix setter')
t.not(npm.bin, npm.localBin, 'bin is not local bin after prefix setter')

mockGlobals(t, { process: { platform: 'win32' } })
t.equal(npm.bin, npm.globalBin, 'bin is global bin in windows mode')
t.equal(npm.dir, npm.globalDir, 'dir is global dir in windows mode')
Expand Down

0 comments on commit 0e74ee4

Please sign in to comment.