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 async middleware #2164

Merged
merged 8 commits into from
May 16, 2022
Merged

Fix async middleware #2164

merged 8 commits into from
May 16, 2022

Conversation

jly36963
Copy link
Contributor

@jly36963 jly36963 commented Apr 10, 2022

Addresses: #2124

Description

The result of applyMiddleware is Arguments | Promise<Arguments>, but the code treats it synchronously. Whenever it returns a promise, that promise isn't handled as such.

This problem is easier to spot when strict is used

Reproduction

const input = 'cmd1 -f Hello -b world'

yargs(input)
  .command(
    'cmd1',
    'cmd1 desc',
    yargs => yargs
      .option('foo', { type: 'string', alias: 'f', required: true })
      .option('bar', { type: 'string', alias: 'b', required: true })
      .middleware(async argv => {
        console.log('sleeping')
        await new Promise(r => setTimeout(r, 2000))
        console.log('waking up')
        return argv
      }, true),
    argv => {
      console.log(argv)
    })
  .strict()
  .parse()

I put a console log after applyMiddleware is called, and I could see that 'waking up' showed up after applyMiddleware finished.

@jly36963 jly36963 requested a review from bcoe April 11, 2022 23:23
@@ -281,6 +281,33 @@ describe('middleware', () => {
}
});

// Addresses: https://github.com/yargs/yargs/issues/2124
// This test will fail if the result of async middleware is not treated like a promise
it('treats result of async middleware as promise', done => {
Copy link
Member

Choose a reason for hiding this comment

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

I would be tempted to write a more specific test to #2124

"Using the strict option in combination with an async middleware, which is applied before the validation, does not result in unknown command"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simplified the test. I kept the done logic, because Mocha will pass this test before the unhandled promise would cause it to fail. If there's a regression, this test will fail because done will get called twice, and one of the executions will have an error passed as an argument

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.

With a couple nits.

'cmd1',
'cmd1 desc',
yargs =>
yargs
Copy link
Member

Choose a reason for hiding this comment

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

I had to read this a few times, I'm wondering if it would be worth simplifying to exactly the minimal case outlined in the regression:

Yargs
  .strict(true)
  .middleware(async () => {}, true)
  .command("test", false, () => {}, () => {})
  .parseAsync(['test']);

I don't have a strong opinion though, if your goal was to exercise parts of the codebase that weren't covered in the above example.

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.

2 participants