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

Ability to do async stuff in check() and coerce() #859

Open
satazor opened this issue Apr 18, 2017 · 10 comments · May be fixed by #1872
Open

Ability to do async stuff in check() and coerce() #859

satazor opened this issue Apr 18, 2017 · 10 comments · May be fixed by #1872

Comments

@satazor
Copy link
Member

@satazor satazor commented Apr 18, 2017

Sometimes you may want to coerce an option but the coercing itself is async. My specific use case was to coerce --config with the output of https://github.com/davidtheclark/cosmiconfig, but this might be handy in other situations.

Supporting this would probably require a lot of work, perhaps the maintainers could weight what would need to be changed. In terms of API, nothing would change, except for the .argv:

const argv = await yargs()
.strict()
.option('config', {
  type: 'string',
  coerce: () => cosmiconfig('my-app'),
})
.argv;

Though, this would be a breaking change. Perhaps, we could have another prop, say .argvPromise to accomplish the same goal.

This thread is to create a discussion about this feature. Feel free to ditch it if it introduces too much complexity.

❤️ yargs

@ljharb
Copy link
Contributor

@ljharb ljharb commented Apr 14, 2018

It seems like yargs could check the .length of the coerce function; if it's 0, expect sync, if it's 1, it will get a callback that's async (and could also await the return value)?

@TomerAberbach
Copy link

@TomerAberbach TomerAberbach commented May 28, 2020

It'd probably still be nice for yargs to support async coerce callbacks natively, but for anyone else who wants a short-term solution, I wrote up the following middleware that allows you to use both sync and async callbacks for coerce (it works because middleware can be async):

const awaitCoercions = argv => Promise.all(
  Object.entries(argv).map(async ([key, value]) => {
    // Awaiting a non-promise just returns the value itself
    argv[key] = await value
  })
)

yargs
  .scriptName('your-script-name')
  // ...
  .middleware([awaitCoercions])
  .argv
@bcoe
Copy link
Member

@bcoe bcoe commented May 29, 2020

@TomerAberbach I've been threatening to do this, for perhaps a year, but I would like to sit down and write a design document for how yargs handles async ... these features have grown organically.

@TomerAberbach
Copy link

@TomerAberbach TomerAberbach commented May 29, 2020

@bcoe Let me know if you need any help! Feel free to ping me on the Slack

@bcoe bcoe added the feature request label Jun 2, 2020
@bcoe
Copy link
Member

@bcoe bcoe commented Jun 2, 2020

@TomerAberbach where things get a little sticky with coerce, is the method is actually applied in yargs-parser.

Which was designed to be synchronous.

How I might approach things, would be to allow coerce to return a promise, and then yargs would need to do post processing, where arguments are awaited before the parsed object is returned.

Where I think we might need a design document, is that I'd be interested in moving towards something like this:

const args = await yargs.parseAsync()

☝️ I think someone should:

  • take stock of the current places where we support async behavior, and make sure we extend on this behavior to work well with the async/await paradigm.
  • look at where we're missing async behavior, and if we extend on the support, isolate it to a method like yargs.parseAsync which returns a Promise, rather than trying to jury-rig it into the existing API.

... maybe we could even consider retiring some of the less thought out async behavior, and isolating it behind this new method.

@TomerAberbach
Copy link

@TomerAberbach TomerAberbach commented Jun 3, 2020

@bcoe I'd be happy to try to put together a design document. I expect to be a bit busy with other things in the next few weeks, but I'll see what work I can fit in. If I don't find the time, then I'll work on it a few weeks from now.

@bcoe
Copy link
Member

@bcoe bcoe commented Jun 3, 2020

@TomerAberbach sounds great 🙌

We're pretty close to the TypeScript conversion, so things might time really well, where we can take on the async problem immediately afterwards.

@Niceplace
Copy link

@Niceplace Niceplace commented Oct 1, 2020

@bcoe Just curious, any idea if we can expect the check function to support async operations anytime soon ? I don't mean to put any pressure, I understand these changes can be complicated to implement I just want an idea of a timeframe to satisfy my own curiosity.

Cheers !

@bcoe
Copy link
Member

@bcoe bcoe commented Oct 1, 2020

@Niceplace I know I'd like to solve the problem in the not too distant future, as with ESM starting to get some traction, import requires async behavior, if we were to try to support command directories in the future.

But, I have almost no time to work on this, and it's a major project, so I'm not sure.

@bcoe bcoe mentioned this issue Dec 6, 2020
10 of 10 tasks complete
@bcoe bcoe added the next-major label Jan 9, 2021
@bcoe
Copy link
Member

@bcoe bcoe commented Jan 9, 2021

I would like to investigate landing this feature as part of the major release being worked on here:

#1823

@bcoe bcoe linked a pull request that will close this issue Feb 20, 2021
0 of 1 task complete
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.

5 participants