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: coerce middleware should be applied once #1978

Merged
merged 8 commits into from Jul 19, 2021
Merged

fix: coerce middleware should be applied once #1978

merged 8 commits into from Jul 19, 2021

Conversation

jly36963
Copy link
Contributor

@jly36963 jly36963 commented Jul 5, 2021

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
Copy link
Member

bcoe commented Jul 8, 2021

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

@jly36963 jly36963 marked this pull request as ready for review July 13, 2021 00:48
Copy link
Member

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

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

test/command.cjs Outdated
yargs =>
yargs.positional('abc', {
type: 'string',
coerce: arg => arg.join(' '),
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

jly36963 commented Jul 15, 2021

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.

@jly36963 jly36963 requested a review from bcoe July 15, 2021 02:57
test/command.cjs Outdated Show resolved Hide resolved
test/command.cjs Outdated Show resolved Hide resolved
There should be no need for the `done()` since your test is assuming sync execution.
@bcoe
Copy link
Member

bcoe commented Jul 15, 2021

@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?

@bcoe bcoe merged commit 14bd6be into yargs:master Jul 19, 2021
@jly36963 jly36963 deleted the fix-coerce-middleware-applied-multiple-times branch July 19, 2021 19:47
@CupOfTea696
Copy link
Contributor

@bcoe yup, that fixes the issue!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants