fix: coerce middleware should be applied once

[jly36963]

Addresses: #1966 and #1802

Debugging

I know that the coerce middleware is being applied twice.
I ran the below example. I logged the execution of the functions that use applyMiddleware, and I logged the type/value of the arg during coercion.

const argv = yargs("rule add foo bar baz")
  .command("rule", "rule desc", (yargs) =>
    yargs.command("add <name> <abc...>", "add desc", (yargs) =>
      yargs.positional("abc", {
        type: "string",
        coerce: (arg) => console.log({ argType: typeof arg, arg }) || arg.join(" "),
      })
    )
  ).argv;

Logs look like this:

parse called
kRunYargsParserAndExecuteCommands called
runCommand called
parseAndUpdateUsage called
kRunYargsParserAndExecuteCommands called
runCommand called
parseAndUpdateUsage called
kRunYargsParserAndExecuteCommands called
pre-validation applyMiddleware called
{ argType: 'object', arg: [ 'bar', 'baz' ] }
post-validation applyMiddleware called
pre-validation applyMiddleware called
{ argType: 'string', arg: 'bar baz' }

The coerce middleware is only supposed to run before validation, but the pre-validation applyMiddleware gets called twice.

Fix

stack trace

get > parse > kRunYargsParserAndExecuteCommands > runCommand > applyMiddlewareAndGetResult > applyMiddleware

The problem

runCommand gets called recursively, which means that the middleware gets applied for each nested command.

Coerce middleware mutates argv, and repeating that mutation is undesired and can easily break.
ie: coerce: (arg) => arg.join()

The solution

I added two properties to the Middleware interface:

• mutates (bool): if the middleware mutates argv and should only be applied once.
• applied (bool): if the mutating middleware has already been applied once.

mutates defaults to false, but is set to true in addCoerceMiddleware.

I then check each middleware during applyMiddleware:

• If it mutates argv:
  • If it hasn't been applied, apply it and set applied to true.
  • If it mutates and has been applied, skip it.
• If it doesn't mutate argv:
  • handle the middleware like it normally would

Feedback

If you have recommendations for a better pattern, or better names for the interface properties I added, let me know. I feel like they could definitely be named better.

[bcoe]

@jly36963 thanks for digging into this one, this would be a great bug to fix. middleware was just overhauled drastically.

[bcoe]

Looking reasonable to me, except I think we could extend the test to more explicitly call out the failure that's happening.

[bcoe]

I think it would be worth extending this function so that it throws if coerce is called more than once.

let executionCount = 0
...
coerce: arg => {
  if (executionCount > 0) throw Error('coerce applied multiple times');
  executionCount++;
  return arg.join(' ');
};

I would then confirm that the test fails prior to your fix.

[jly36963]

I made some changes to the unit test, and this is the result:

• Fails if the fix is not implemented (tested on master branch)
• Passes if the fix is implemented (tested on this branch)

I intentionally did not put the assertions / done() inside a command handler. When I put the assertions/done in the handler and tested on master branch, The test passed when it should have failed. This is what the logs looked like:

• coerce middleware called
• handler called
• ✓ should not be applied multiple times for nested commands
• coerce middleware called
• fail callback called

done() had already been called before the second execution of coerce would have caused it to fail.

[bcoe]

@andymcintosh @CupOfTea696 I've published @jly36963's fix to next, could you give this a try:

npm i yargs@next.

And let us know if it fixes the issues you were bumping into?

[CupOfTea696]

@bcoe yup, that fixes the issue!