fix: properties accessed on singleton now reflect current state of instance #1366
Conversation
singletonify() used to set Argv's properties to the singleton's properties values _at the time of Argv's last call_. So Argv's properties and the singleton's ones diverged afterwards, such as parsed after a call to parse, still false in Argv.parsed, not in singleton.parse. singletonify() now adds setters to Argv instead, looking for the value of the corresponding property in the singleton _at the time the property is used_.
There was a problem hiding this comment.
This seems like a pretty reasonable fix...
On my mind regarding #1137 however, is...
Could we figure out a way to make singleton mode safer (perhaps as soon as someone calls parse() or argv(), as an example, we re-initialize the singleton as if someone had created a new yargs instance?).
Part of me wishes we could retire the singleton API, but it's very widely used... perhaps the next best thing would be trying to eliminate state.
| @@ -25,8 +25,10 @@ function singletonify (inst) { | |||
| Object.keys(inst).forEach((key) => { | |||
| if (key === 'argv') { | |||
| Argv.__defineGetter__(key, inst.__lookupGetter__(key)) | |||
| } else if (typeof inst[key] === 'function') { | |||
| Argv[key] = inst[key].bind(inst) | |||
| } else { | |||
There was a problem hiding this comment.
why don't we make this slightly more explicit:
if (['$0', 'parsed'].indexOf(key) !== -1) {
and we should be mindful about leaking any more stateful variables on the yargs object going forward.
There was a problem hiding this comment.
I don't get the point. If we support singleton mode, shouldn't every property/method call on Argv have the same effect as if directly called on the singleton?
There was a problem hiding this comment.
fair 👍 I just noticed that, I think, argv, $0, and parsed are the only properties that leak ... and honestly, I don't think we meant to leak parsed as part of the API.
There was a problem hiding this comment.
Oops! I did not realize this... However, parsed is already leaking from the API when not using the singleton. So, does it have a sense to filter properties we accept to leak from the singleton, and not to do it when not using the singleton?
| @@ -837,6 +837,17 @@ describe('yargs dsl tests', () => { | |||
| }) | |||
| }) | |||
|
|
|||
| describe('parsed', () => { | |||
| it('should be false before parsing', () => { | |||
There was a problem hiding this comment.
I wonder if we could figure out the specific exception that was happening with showHelp and put a test around this?
There was a problem hiding this comment.
I agree. A simple "showHelp should not fail" should do the job
|
@mleguen what are your feelings regarding us eventually fixing yargs, such that you can use it in singelton mode, but call I'm happy to land this fix, but think we might want to have the above goal eventually in mind? |
|
@bcoe We are trying to deal here with 3 different issues:
My opinion is: we should do 1 and 2 in this PR, and 3 in another one (I should be able to spend some time on it next Wednesday). |
mleguen
changed the title
bcoe
changed the title
singletonify()used to setArgv's properties toinst's properties values at the time ofArgv's last call.So
Argv's properties and theinst's ones could diverge afterwards, such asparsedafter a call toparse(), still beingfalseinArgv.parsed, not ininst.parse.singletonify()now adds setters toArgvinstead, looking for the value of the corresponding property ininstat the time the property is used.