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

Promote the non-singleton interface, require('yargs/yargs'), as the default #1648

Closed
roryokane opened this issue May 3, 2020 · 4 comments · Fixed by #1768
Closed

Promote the non-singleton interface, require('yargs/yargs'), as the default #1648

roryokane opened this issue May 3, 2020 · 4 comments · Fixed by #1768
Labels

Comments

@roryokane
Copy link
Contributor

roryokane commented May 3, 2020

Background: problems with the singleton interface

When I set up Yargs in my command-line app, I learned how to use Yargs from two places: the main documentation page at https://yargs.js.org/docs/ and the examples in docs/examples.md, which I found by searching the web for “yargs example”. According to these instructions, I imported Yargs with import * as yargs from 'yargs', the TypeScript equivalent of require('yargs'). This 'yargs' import, I later learned, is called Yargs’s singleton interface.

While following the instructions in those places, I experienced two bugs in my app that were ultimately caused by this imported yargs object having global state. A summary of these two problems, if you’re curious:

  • Problem: Tests of my command-line parsing with .parse() were failing due to state shared from previous test runs.
    • Solution: Call yargs.reset() before adding flags to yargs so it resets before each test.
  • Problem: Yargs was acting like I never passed it .options and other methods, but only when showing usage with --help, not when showing usage due to an argument that failed validation.
    • Solution: Set "esModuleInterop": false in my tsconfig.json for TypeScript (this solution took a long time to track down.)

While searching the Yargs issue tracker for another issue, I found a reference to an alternative to .reset(): the Yargs non-singleton interface, accessed with require('yargs/yargs').

The documentation I found for it (in Advanced Topics) is slim and didn’t answer all of my questions, but I managed to switch my app to use the non-singleton interface instead of plain require('yargs'). After undoing the two previously-mentioned solutions for the state-related problems I experienced, my app still seems to work. I can also see that my code using require('yargs/yargs') is no more complicated than the code I had using require('yargs') – all I had to change was adding () to the result of the import.

Arguments for the non-singleton interface

My success with the non-singleton interface makes me think that if the documentation had explained and promoted the non-singleton interface from the start, in https://yargs.js.org/docs/ and other places, I would never have encountered those two bugs.

I don’t see any pattern of use that the singleton interface allows and the non-singleton interface doesn’t allow. The singleton interface does make it slightly easier to spread out specification of Yargs commands across different files, but that isn’t much harder to do with the non-singleton interface. That pattern seems likely to lead to confusing code anyway, and so it’s not a good default to encourage.

Also, in general, I would call global state a code smell – a property of hard-to-understand programs. It should only be used when the convenience makes up for the increased likelihood of bugs. I see the singleton interface’s enabled convenience as being very minor and its enabled bugs as being serious.

Question

Is there a reason I am missing to promote the singleton interface as the default, or should we change the documentation to promote the non-singleton interface as the default? I could contribute some of those documentation changes.

The singleton interface should probably still be mentioned in the API documentation and in Advanced Topics, but it should be clearly labeled as only for the sake of those with programs already written using that interface.

@roryokane roryokane changed the title Promote the non-singleton interface (require('yargs/yargs')) as the default? Promote the non-singleton interface, require('yargs/yargs'), as the default? May 4, 2020
@roryokane
Copy link
Contributor Author

Related: I see that #1360 (comment) mentioned the idea of dropping the singleton interface in a future version of Yargs. That idea wasn’t discussed further in that thread though.

@roryokane
Copy link
Contributor Author

roryokane commented May 7, 2020

If you want more detail about the first of the two problems I had with the yargs import:

  • Problem: Tests of my command-line parsing with .parse() were failing due to state shared from previous test runs.
    • Solution: Call yargs.reset() before adding flags to yargs so it resets before each test.

See my explanation and reproduction steps in #1639 (comment).

If you want more detail about the second of the two problems:

  • Problem: Yargs was acting like I never passed it .options and other methods, but only when showing usage with --help, not when showing usage due to an argument that failed validation.
    • Solution: Set "esModuleInterop": false in my tsconfig.json for TypeScript (this solution took a long time to track down.)

See commit eb9f4a9 in my project, “Disable esModuleInterop in TypeScript config to fix Yargs-related bug”.

@mleguen
Copy link
Member

mleguen commented May 7, 2020

I am personally not fond of the singleton API, which I do not use. So I think promoting the regular API as the standard one, as a step towards deprecating then removing the singleton API in a future version, is a good idea.

@roryokane roryokane changed the title Promote the non-singleton interface, require('yargs/yargs'), as the default? Promote the non-singleton interface, require('yargs/yargs'), as the default Jul 12, 2020
@bcoe bcoe added docs and removed feature request labels Sep 25, 2020
@bcoe
Copy link
Member

bcoe commented Sep 25, 2020

@roryokane @mleguen I agree that we should push folks towards the non-singleton interface, this is the default for the new ESM support we recently added to yargs:

import yargs from 'yargs'
import { hideBin } from 'yargs/helpers'

yargs(hideBin(process.argv))
  .command('curl <url>', 'fetch the contents of the URL', () => {}, (argv) => {
    console.info(argv)
  })
  .demandCommand(1)
  .argv

I would be quite open to an update to docs that demonstrated the use of yargs() rather than yargs..

Any interested in contributing?

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

Successfully merging a pull request may close this issue.

3 participants