Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: clean up npm object #7416

Merged
merged 1 commit into from
Apr 25, 2024
Merged

fix: clean up npm object #7416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 11 additions & 29 deletions lib/npm.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading