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.

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

fix: remove unused npm.setCmd method #7415

Merged
merged 2 commits into from
Apr 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 1 addition & 4 deletions lib/cli/entry.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,12 @@ module.exports = async (process, validateEngines) => {
// Now actually fire up npm and run the command.
// This is how to use npm programmatically:
try {
const { exec } = await npm.load()
const { exec, command, args } = await npm.load()

if (!exec) {
return exitHandler()
}

const command = npm.argv.shift()
const args = npm.argv

if (!command) {
output.standard(npm.usage)
process.exitCode = 1
Expand Down
11 changes: 4 additions & 7 deletions lib/npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ class Npm {
return this.constructor.version
}

setCmd (cmd) {
// Call an npm command
async exec (cmd, args = this.argv) {
const Command = Npm.cmd(cmd)
const command = new Command(this)

Expand All @@ -88,12 +89,6 @@ class Npm {
process.env.npm_command = this.command
}

return command
}

// Call an npm command
async exec (cmd, args = this.argv) {
const command = this.setCmd(cmd)
return time.start(`command:${cmd}`, () => command.cmdExec(args))
}

Expand All @@ -102,6 +97,8 @@ class Npm {
const { exec = true } = await this.#load().then(r => r ?? {})
return {
exec,
command: this.argv.shift(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not to be solved here but this .shift() smells

args: this.argv,
}
})
}
Expand Down
17 changes: 5 additions & 12 deletions test/fixtures/mock-npm.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ const setupMockNpm = async (t, {
// preload a command
command = null, // string name of the command
exec = null, // optionally exec the command before returning
setCmd = false,
// test dirs
prefixDir = {},
homeDir = {},
Expand Down Expand Up @@ -242,7 +241,11 @@ const setupMockNpm = async (t, {
init,
load,
mocks: withDirs(mocks),
npm: { argv, excludeNpmCwd: true, ...withDirs(npmOpts) },
npm: {
argv: command ? [command, ...argv] : argv,
excludeNpmCwd: true,
...withDirs(npmOpts),
},
})

if (config.omit?.includes('prod')) {
Expand All @@ -267,16 +270,6 @@ const setupMockNpm = async (t, {
const mockCommand = {}
if (command) {
const Cmd = mockNpm.Npm.cmd(command)
if (setCmd) {
// XXX(hack): This is a hack to allow fake-ish tests to set the currently
// running npm command without running exec. Generally, we should rely on
// actually exec-ing the command to asserting the state of the world
// through what is printed/on disk/etc. This is a stop-gap to allow tests
// that are time intensive to convert to continue setting the npm command
// this way. TODO: remove setCmd from all tests and remove the setCmd
// method from `lib/npm.js`
npm.setCmd(command)
}
mockCommand.cmd = new Cmd(npm)
mockCommand[command] = {
usage: Cmd.describeUsage,
Expand Down
13 changes: 12 additions & 1 deletion test/lib/utils/reify-output.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,18 @@ const mockReify = async (t, reify, { command, ...config } = {}) => {
const mock = await mockNpm(t, {
command,
config,
setCmd: true,
})

// Hack to adapt existing fake test. Make npm.command
// return whatever was passed in to this function.
// What it should be doing is npm.exec(command) but that
// breaks most of these tests because they dont expect
// a command to actually run.
Object.defineProperty(mock.npm, 'command', {
get () {
return command
},
enumerable: true,
})

reifyOutput(mock.npm, reify)
Expand Down
Loading