Skip to content

Fix async middleware#2164

Merged
bcoe merged 8 commits intoyargs:mainfrom
jly36963:fix-async-middleware
May 16, 2022
Merged

Fix async middleware#2164
bcoe merged 8 commits intoyargs:mainfrom
jly36963:fix-async-middleware

Conversation

@jly36963
Copy link
Contributor

@jly36963 jly36963 commented Apr 10, 2022 •
edited
Loading

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
bcoe
bcoe reviewed Apr 21, 2022
View reviewed changes
test/middleware.cjs Outdated

// 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

@bcoe bcoe Apr 21, 2022

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

@jly36963 jly36963 Apr 21, 2022

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

bcoe
bcoe approved these changes Apr 21, 2022
View reviewed changes
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.

bcoe
bcoe reviewed Apr 21, 2022
View reviewed changes
test/middleware.cjs Outdated
'cmd1',
'cmd1 desc',
yargs =>
yargs
Copy link
Member

@bcoe bcoe Apr 21, 2022

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.

@bcoe bcoe merged commit cbc2eb7 into yargs:main May 16, 2022
@github-actions github-actions bot mentioned this pull request May 16, 2022
Merged
This was referenced Sep 6, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bcoe bcoe bcoe approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@jly36963 @bcoe