Fix breaking error that occurs when a command is called programatically with both an argument array and a callback. #2591

Closed
wants to merge 1 commit into
from

3 participants

@ghost

Previously, calling a command programatically with both an array of arguments and a callback, such as:

npm.uninstall([ 'step', 'npm' ], function (err, data) {  });

Would result in something like:

npm WARN uninstall invalid argument: ["step","npm"]

While the same operation, with the callback omitted, such as:

npm.uninstall([ 'step', 'npm' ], function (err, data) {  });

Would succeed without error.

For command calls, npm checks to see if there is only one argument present, and if so, if said argument is an array. If this check passes, npm considers this array to be the list of arguments for the command. Specifying a callback as well a command array, however, breaks this logic, because there are multiple arguments specified in the function call.

Following this check is another check to see if a callback is specified. If this check passes, the callback is popped off the arguments array, which, in the case described above, would leave it with one element only: the array of arguments to pass to the command. Simply swapping the order of these checks corrects the issue here.

I have, however, not been able to get npm's tests to run properly, even on an unmodified clone from isaacs/npm. Someone else, then, would have to verify that this change doesn't break anything else. UPDATE: See below.

Bug appears to have been introduced around d2e23de.

Michael Smith Fix error if command called with args array and cb 27b7147
@ghost

After applying the ENAMETOOLONG fix in #2592, I've been able to get the tests running, and this patch passes successfully.

@robertkowalski
npm member

Hi @mintplant - and sorry for the late answer!

Why would one omit the callback in the async API npm has?

For me it seems that passing a callback is mandatory in npm, and omitting it was possible by accident, when I have a look on the code and API docs ( https://npmjs.org/api/commands.html and e.g. https://npmjs.org/api/search.html )

@timoxley
npm member

-1 adding another calling style (complexity) for minimal benefit.

Restricting it to "just" 2 ways of invoking the command (cmd([arg1, arg2], cb) & cmd(arg1, arg2, cb)) is enough IMO, if you want to add more convenience you can easily add it in your own wrapping code @mintplant?

@othiym23

I agree with @timoxley, with the further note that the API is probably going to be seeing some fairly dramatic changes in the not-too-distant future, and making assumptions about the future shape of the API is really not a safe idea.

Thanks for the time you spent putting this together, @mintplant!

@othiym23 othiym23 closed this Feb 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment