.check() method doesn't clear checks array when exiting or completing

[jpbriggs408]

Overview
It is my understanding that one should be able to chain calls to .check(). However, it appears that this is not the case, and that the scope never clears. This seems to be due to a let checks = []; that is declared in the module scope, effectively turning it into a static array.

Context
In working on an update to our build system, I added a new CLI option. We need to enforce that this flag is only enabled under the condition that two other flags are also enabled, and fail otherwise. I learned of the .check() method in looking through documentation, and it looked like it was perfect for this use case.

When putting out my PR, I learned that we have additional validation checks elsewhere in our code, and for consistency, my team agreed that moving the existing checks into the .check() method made a lot of sense. However, upon simply cut-and-pasting the existing validation logic that we had into check, our tests immediately began to fail.

It seemed almost like the check function was "stuck" in previous tests, as later tests were failing the .check()with errors and scope of tests that had already been completed. More specifically, a parameter args that we had seemed to be different inside vs. outside of the scope of .check(). Here is a helpful screenshot that my teammate annotated, which includes the failing validation logic that was cut-and-pasted in:

Further, it appears that renaming this variable to anything else did not help (and so there wasn't a naming conflict or something like that), setting args to a new variable on the global Node scope and using that didn't work, and further that all variables aside from the argv passed in were outdated.

So after a lot of debugging, we dove into the source code and discovered these functions here, which seems to handle calls to .check().

(^ The debugger statement here is from me.)

It appears that the checks array does not clear after a test that intentionally throws an error, for instance, and that this would therefore have implications for chained calls to .check() inheriting from the previous ones, and additionally have implications for testing environments.

Steps to Reproduce:
Please let me know if you would like more specific steps to reproduce this or if you can get a working example from the context above instead.

Notes:
We use Jest for testing and we are on yargs 16.2.0.

[bcoe]

@jpbriggs408 I'm in the process of overhauling how check and coerce work, so that they both share the middleware implementation.

If you get a moment would love you to try out #1872 (make sure it works for your complex use case). You can install it via npm i yargs@next.

However ...

You might still bump into the bug you're describing. Calling yargs directly is a singleton, so can leak between invocations:

yargs
  .config()
  .parse();

I recommend using the following approach instead:

yargs()
  .config()
  .parse('arguments')

Invoking yargs() returns a new instance of yargs, so will not leak between test cases.

[jpbriggs408]

@bcoe thanks for the response! That seems like a great change, and I am looking forward to seeing that make its way to a release. Unfortunately, #1872 does not work for us. However, using the other approach that you mentioned does indeed work! Thank you very much for the help!

[bcoe]

@jpbriggs408 👍 what problems did #1872 cause?

[jpbriggs408]

@bcoe no new problems, but also did not fix the failing tests. Stepping into the debugger indicates that, as before, the same intentionally failing case finds its way into later tests.