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

feat: allow parse with no arguments as alias for yargs.argv #944

Merged
merged 1 commit into from Sep 3, 2017

Conversation

bcoe
Copy link
Member

@bcoe bcoe commented Aug 25, 2017

I showed the the proposed promise API for yargs to several coworkers today at npm, and they managed to talk me out of it. Their reasoning being:

  1. yargs is for the most part a synchronous API, and they didn't see much value in introducing the deferred behavior.
  2. my coworker @zkat felt that the method name .then() felt weird; this method name should be used internally by the Promise api, not by APIs exposing a promise.

Rather than introducing a new method run() (or the method I recommended then() for exposing a Promise), I feel a good compromise is to instead extend the method parse() which already exists in the API. Calling:

require('yargs')
  .option('x', {describe: 'x option'})
  .parse()

is now possible, and is equivalent to:

require('yargs')
  .option('x', {describe: 'x option'})
  .argv

I like this approach: it gets around the linting warning now frequently triggered by .argv, and avoids adding a new method to yargs' already large API surface.

We definitely might revisit a Promise-based API in the future, one recommendation made by my coworker @chrisdickinson was to make yargs' command API promise based, e.g.,

  • a command would return a promise to execute.
  • yargs would return a promise that resolves once the command is completed.

@bernardmcmanus
Copy link

👍 this feels much cleaner

@bcoe bcoe merged commit a9f03e7 into master Sep 3, 2017
@bcoe bcoe deleted the parse-with-no-args branch September 3, 2017 19:18
@bcoe
Copy link
Member Author

bcoe commented Sep 3, 2017

@bernardmcmanus going to do my best to get this out today.

@bcoe
Copy link
Member Author

bcoe commented Sep 3, 2017

@bernardmcmanus I've released this work in yargs@next would love your help testing if you have a few moments:

npm i yargs@next

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.

None yet

2 participants