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

Allow command handler to accept a promise #510

Closed
vjpr opened this issue May 19, 2016 · 14 comments
Closed

Allow command handler to accept a promise #510

vjpr opened this issue May 19, 2016 · 14 comments

Comments

@vjpr
Copy link

@vjpr vjpr commented May 19, 2016

This would allow much easier use of ES7 async/await.

yargs.command('ls', 'List', async function() { ... })

Workaround

Use IIFE.

yargs.command('ls', 'List', function () { (async function() { ... })().then() })
@silenceisgolden
Copy link

@silenceisgolden silenceisgolden commented Apr 3, 2017

Could this be accomplished using gulp's async-done library? If so I can try to take a stab at a PR. Thanks!

@bcoe
Copy link
Member

@bcoe bcoe commented Apr 3, 2017

@silenceisgolden I think we should be able to pull this off without pulling in any dependencies; once the command itself is invoked, its now the application's responsibility to handle things -- so it's perfectly fine for the handler to be async.

I think, basically, we'd just need to update the argument validation and invocation of a command handler to understand a promise. I love this idea (thanks @vjpr) and would love your help with the patch @silenceisgolden.

@silenceisgolden
Copy link

@silenceisgolden silenceisgolden commented Apr 3, 2017

@bcoe Sounds good, I'll see what I can put together.

@eddywashere
Copy link

@eddywashere eddywashere commented May 18, 2017

I'd like to see this as well. For now I'm getting by with this https://github.com/eddywashere/yargs-promise.

@satazor
Copy link
Member

@satazor satazor commented Jul 25, 2017

I also ended up doing an user-land solution for this problem just like @eddywashere, but the solution integrates with yargs, instead of changing its internals.

https://github.com/moxystudio/yargs-promise-handler

@ianwremmel
Copy link

@ianwremmel ianwremmel commented Jul 29, 2017

This also seems to get the job done without needing to modify every command builder with a wrapper. That said, native support would be nice.

process.on('unhandledRejection', function onUnhandledRejection(err) {
  var cmd = process.argv.slice(2).join(' ');
  console.error('Failed to execute "' + cmd + '"');
  console.error(err.toString());
  if (argv.debug) {
    console.error(err);
  }
  // eslint-disable-next-line no-process-exit
  process.exit(1);
});
@zenflow
Copy link
Contributor

@zenflow zenflow commented Nov 1, 2017

To those subscribed: I've submitted PR #1001 to address this. Reviews are appreciated.

@quilicicf
Copy link

@quilicicf quilicicf commented Jan 5, 2018

Hi, I've tested the 10.1.0 with a command like this:

.command([ 'replicate', 'r' ], 'Clones a repository', async (_yargs) => require('./lib/git/replicate').replicate(_yargs)))

When the function replicate contains an await statement, the parsing fails with this error:

~/forge/Gut/node_modules/yargs/yargs.js:1100
      else throw err
           ^

TypeError: innerYargs._parseArgs is not a function
    at Object.runCommand (~/forge/Gut/node_modules/yargs/lib/command.js:195:45)
    at Object.parseArgs [as _parseArgs] (~/forge/Gut/node_modules/yargs/yargs.js:1013:30)
    at Object.get [as argv] (~/forge/Gut/node_modules/yargs/yargs.js:957:21)
    at Object.<anonymous> (~/forge/Gut/index.js:50:3)
    at Module._compile (module.js:638:14)
    at Object.Module._extensions..js (module.js:652:10)
    at Module.load (module.js:560:32)
    at tryModuleLoad (module.js:503:12)
    at Function.Module._load (module.js:495:3)
    at Function.Module.runMain (module.js:682:10)

Am I doing something wrong ?

@zenflow
Copy link
Contributor

@zenflow zenflow commented Jan 5, 2018

@quilicicf The third argument is the command builder, not the command handler https://github.com/yargs/yargs/blob/master/docs/api.md#commandcmd-desc-builder-handler
Insert a noop (i.e. () => {}) for the third argument

@quilicicf
Copy link

@quilicicf quilicicf commented Jan 8, 2018

Thanks @zenflow!
I was confused by reading the feature request because the async function is in third position there.
Actually, I'd need the builder to be async in my use case: I have argument defaults that depend on a global configuration file. If the configuration file does not exist, the user is asked to fill the configuration then the initial command proceeds.

I don't know if my use case is one that yargs should support or if I should simplify things a bit.

In any case, it could be good to change the initial comment to show that it's the command handler (4th position) that's been changed ?

@zenflow
Copy link
Contributor

@zenflow zenflow commented Jan 9, 2018

@quilicicf

I was confused by reading the feature request because the async function is in third position there. [...]

I did not notice that, but you're right! If @vjpr (or someone else with the permission) sees this, could you please correct the examples in the issue description for posterity? I.e. Insert a noop () => {} for the third argument since that is the command builder, not the handler.

Actually, I'd need the builder to be async in my use case [...]

It will be more complicated to make the command builder async. But you should open an issue for it!

Is the new feature for async command handlers in v10.1.0 working ok for you? Some feedback over on the PR would be greatly appreciated! #1001 (comment)

@quilicicf
Copy link

@quilicicf quilicicf commented Jan 9, 2018

Well I don't use it in my use case but I can test it if that helps.
I'll open an issue for the command builder too, thanks!

@bcoe
Copy link
Member

@bcoe bcoe commented Nov 9, 2019

we support async functions in command handlers now, we won't likely be adding this functionality to builders, however we are talking about making an async version of yargs:

see: #1453

@bcoe bcoe closed this Nov 9, 2019
@quilicicf
Copy link

@quilicicf quilicicf commented Nov 12, 2019

Not the expected outcome but damn, I'm excited about it.

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

Successfully merging a pull request may close this issue.

None yet
9 participants