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

Explain in docs why .reset() is deprecated, or un-deprecate it #1639

Closed
roryokane opened this issue May 3, 2020 · 5 comments
Closed

Explain in docs why .reset() is deprecated, or un-deprecate it #1639

roryokane opened this issue May 3, 2020 · 5 comments
Labels

Comments

@roryokane
Copy link
Contributor

roryokane commented May 3, 2020

Documentation problem

The .reset() API documentation on https://yargs.js.org/docs/ starts like this:

.reset() [DEPRECATED]

Reset the argument object built up so far. This is useful for creating nested command line interfaces. Use global to specify keys that should not be reset.

It doesn’t explain why .reset() is deprecated or what to use instead. This is bad because it’s the only option I see for my use-case, but it leaves me wondering if some bug will eventually bite me.

My use-case for .reset() is unit tests: my unit tests of my CLI arg parsing sometimes fail when I try to parse args multiple times due to global state that persists between tests, so I have to call yargs.reset() before each test.

Possible resolutions

It looks like the file docs/api.md is the source of https://yargs.js.org/docs/. That’s the file that should be updated.

I see one related closed issue, #654. Reading that issue, I can guess that maybe you added the deprecation warning when that issue was created but forgot to delete it when that issue was closed.

The first comment in that issue also mentions a non-singleton API require('yargs/yargs') as an alternative. Right now https://yargs.js.org/docs/ doesn’t mention that API anywhere and I’m not sure if it still exists (the comment mentioning it was written in 2016). If it still exists, that API would be perfect for my use-case – even better than .reset() – and it should be mentioned in .reset()’s documentation, whether or not .reset() stays deprecated. (That non-singleton API should also be mentioned in the API section at the top of the documentation, though this can be a separate issue.)

@mleguen
Copy link
Member

mleguen commented May 6, 2020

global state that persists between tests

This should not be the case: could you please give us some code reproducing this?

my unit tests of my CLI arg parsing sometimes fail when I try to parse args multiple times [...], so I have to call yargs.reset() before each test.

Did you try building a fresh yargs instance before each test, instead of reusing and resetting the same instance each time?

@roryokane
Copy link
Contributor Author

roryokane commented May 7, 2020

could you please give us some code reproducing this [global state that persists between tests]?

Sure: it’s the state of the code after this commit of my open-source project. In that commit, you can see I added a yargs.reset() call to cli_arg_parsing.ts. If you remove it locally, you will see failures when running npm test (which calls the Jest test runner).

How to reproduce locally:

  1. git clone https://github.com/roryokane/Transcribe-xsc-file-converter
  2. cd Transcribe-xsc-file-converter/
  3. git checkout 959b841a94effd843826606e34f617a8bb978bf7
  4. npm install
  5. npm test – all tests pass
  6. comment out the yargs.reset() call in cli_arg_parsing.ts
  7. npm test – some tests in ./cli_arg_parsing.test.ts fail

The relevant parts of the code:

cli_arg_parsing.ts

import * as yargs from "yargs"
  yargs.reset()

  const yargsParser = yargs
    .usage("$0 [option] [file]")
    // … examples and options …
    .exitProcess(options.outputAndExitOnError)
    .check((argv) => {
      const filePaths = argv._
      if (filePaths.length > 1) {
        throw new Error(
          "Multiple files were given as arguments, but only one file can be converted at a time. File paths: " +
            filePaths.join(",")
        )
      }
      return true
    })
    .version()
    .help()

  let yargsArgv
  if (options.outputAndExitOnError) {
    yargsArgv = yargsParser.parse(argv)
  } else {
    yargsArgv = yargsParser.parse(argv, (err: Error | undefined, _argv: Object, _output: string) => {
      if (err) throw err
    })
  }

cli_arg_parsing.test.ts

import { parseArgv } from "./cli_arg_parsing"

function parseArgvInTestMode(argv: Array<string>) {
  return parseArgv(argv, { outputAndExitOnError: false })
}
test("rejects unknown format", () => {
  expect(() => {
    parseArgvInTestMode(["--format", "transcribe_version_999"])
  }).toThrow()
})

// Yargs threw an error in the above test, as expected

test("accepts one file", () => {
  expect(parseArgvInTestMode(["some file with spaces.xsc"])).toEqual({
    inputSource: {
      filePath: "some file with spaces.xsc",
      type: "file",
    },
    outputFormat: "generic",
  })
})

// Yargs throws the same error as in the previous test.
// Therefore, Yargs has global state that tracks whether it encountered an error,
// and this state persists even after parsing a different array of args.

Did you try building a fresh yargs instance before each test, instead of reusing and resetting the same instance each time?

I’m not sure what you mean by “building a fresh yargs instance” – the docs page doesn’t suggest any way to do that. The result of require('yargs') seems to be the only instance you get. This is another example of global state: you have one global Yargs, and then each call to .example, .option, etc. mutates it.

If you’re referring to require('yargs/yargs'), I didn’t know that was an option when I was first exploring this issue, because the docs page never mentions it. I only found about it later, when searching the issue tracker to report issues like these.

After finding out about yargs/yargs from the issue tracker, in commit fe1d86e I changed my code to use it, by importing it with import yargs from "yargs/yargs" and removing the .reset() call. The tests still passed, so yargs/yargs indeed seemed to be an alternative to .reset(). Since I liked the non-global-state solution of yargs/yargs more than .reset(), I made a separate issue about the lack of documentation for yargs/yargs (the failing tests problem explained above is the first of the two problems I mentioned in that issue).

@mleguen
Copy link
Member

mleguen commented May 7, 2020

I’m not sure what you mean by “building a fresh yargs instance”

I mean using yargs as a function, either required from yargs:

const yargs = require('yargs')
const yargsParser = yargs()

or yargs/yargs:

const yargs = require('yargs/yargs')
const yargsParser = yargs()

which would return a new yargs instance, instead of using it as an object:

const yargs = require('yargs')
const yargsParser = yargs

which is always the same instance, initialized the first time the module is loaded (the so-called singleton mentioned in the docs - but maybe cryptically - here: https://github.com/yargs/yargs/blob/master/docs/advanced.md#using-the-non-singleton-interface).

@shadowspawn
Copy link
Member

For interest, I came across another mention of decreased reliance on reset: #1895

@shadowspawn
Copy link
Member

The non-singleton API to yargs is now the primary method used in the documentation, and is the recommended approach for multiple parsing situations like unit tests. i.e. Make a fresh yargs instance for each test.

.reset() is now gone from documentation. Deprecation done!

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

No branches or pull requests

4 participants