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.

Sign up for GitHub

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

Jump to bottom

Fix async middleware #2164

Merged
merged 8 commits into from May 16, 2022
Merged

Fix async middleware #2164

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
65cb6a7
fix: handle result of async middleware
jly36963 Apr 10, 2022
6a99fb1
cleanup
jly36963 Apr 10, 2022
1d35513
Merge branch 'main' into fix-async-middleware
jly36963 Apr 10, 2022
bef63e2
Merge branch 'main' into fix-async-middleware
jly36963 Apr 21, 2022
fca98f4
Simplify test
jly36963 Apr 21, 2022
0d8d0d4
Merge branch 'main' into fix-async-middleware
bcoe Apr 25, 2022
1d829e8
Merge branch 'main' into fix-async-middleware
jly36963 May 15, 2022
1c2718f
Merge branch 'main' into fix-async-middleware
bcoe May 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
86 changes: 63 additions & 23 deletions lib/command.ts
View file
Open in desktop
Expand Up @@ -364,34 +364,16 @@ export class CommandInstance {
pc.push(c);
return `$0 ${pc.join(' ')}`;
}
private applyMiddlewareAndGetResult(
private handleValidationAndGetResult(
isDefaultCommand: boolean,
commandHandler: CommandHandler,
innerArgv: Arguments | Promise<Arguments>,
currentContext: Context,
helpOnly: boolean,
aliases: Dictionary<string[]>,
yargs: YargsInstance
): Arguments | Promise<Arguments> {
let positionalMap: Dictionary<string[]> = {};
// If showHelp() or getHelp() is being run, we should not
// execute middleware or handlers (these may perform expensive operations
// like creating a DB connection).
if (helpOnly) return innerArgv;
if (!yargs.getInternalMethods().getHasOutput()) {
positionalMap = this.populatePositionals(
commandHandler,
innerArgv as Arguments,
currentContext,
yargs
);
}
const middlewares = this.globalMiddleware
.getMiddleware()
.slice(0)
.concat(commandHandler.middlewares);
innerArgv = applyMiddleware(innerArgv, yargs, middlewares, true);

yargs: YargsInstance,
middlewares: Middleware[],
positionalMap: Dictionary<string[]>
) {
// we apply validation post-hoc, so that custom
// checks get passed populated positional arguments.
if (!yargs.getInternalMethods().getHasOutput()) {
Expand Down Expand Up @@ -453,6 +435,64 @@ export class CommandInstance {

return innerArgv;
}
private applyMiddlewareAndGetResult(
isDefaultCommand: boolean,
commandHandler: CommandHandler,
innerArgv: Arguments,
currentContext: Context,
helpOnly: boolean,
aliases: Dictionary<string[]>,
yargs: YargsInstance
): Arguments | Promise<Arguments> {
let positionalMap: Dictionary<string[]> = {};
// If showHelp() or getHelp() is being run, we should not
// execute middleware or handlers (these may perform expensive operations
// like creating a DB connection).
if (helpOnly) return innerArgv;
if (!yargs.getInternalMethods().getHasOutput()) {
positionalMap = this.populatePositionals(
commandHandler,
innerArgv as Arguments,
currentContext,
yargs
);
}
const middlewares = this.globalMiddleware
.getMiddleware()
.slice(0)
.concat(commandHandler.middlewares);

const maybePromiseArgv = applyMiddleware(
innerArgv,
yargs,
middlewares,
true
);

return isPromise(maybePromiseArgv)
? maybePromiseArgv.then(resolvedInnerArgv =>
this.handleValidationAndGetResult(
isDefaultCommand,
commandHandler,
resolvedInnerArgv,
currentContext,
aliases,
yargs,
middlewares,
positionalMap
)
)
: this.handleValidationAndGetResult(
isDefaultCommand,
commandHandler,
maybePromiseArgv,
currentContext,
aliases,
yargs,
middlewares,
positionalMap
);
}
// transcribe all positional arguments "command <foo> <bar> [apple]"
// onto argv.
private populatePositionals(
Expand Down
27 changes: 27 additions & 0 deletions test/middleware.cjs
View file
Open in desktop
Expand Up @@ -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

const input = 'cmd1 -f Hello -b world';
yargs(input)
.command(
'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.

.option('foo', {type: 'string', alias: 'f', required: true})
.option('bar', {type: 'string', alias: 'b', required: true})
.middleware(async argv => {
await new Promise(r => setTimeout(r, 5));
return argv;
}, true),
_argv => {
done();
}
)
.fail(msg => {
done(new Error(msg));
})
.strict()
.parse();
});

it('runs before validation, when middleware is added in builder', done => {
yargs(['mw'])
.command(
Expand Down