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

Review boolean declaration option parsing #885

Closed
aciccarello opened this issue Oct 18, 2018 · 1 comment
Closed

Review boolean declaration option parsing #885

aciccarello opened this issue Oct 18, 2018 · 1 comment

Comments

@aciccarello
Copy link
Collaborator

In PR #845, a question came up about the parsing of boolean declaration options.

case ParameterType.Boolean:
value = (typeof value === void 0 ? true : !!value);
break;

The original code was written to return true if the option is undefined and the truthy value of the option otherwise. It was noted by @Gerrit0 that typeof will always return a string, therefore the typeof check will always return false and the expression should be simplified to value = !!value;.

Since there is likely a bug in this code, it should be thoroughly reviewed and corrected.

@Gerrit0
Copy link
Collaborator

Gerrit0 commented Dec 26, 2019

I spent some time looking into this and have determined that either value === undefined ? true : !!value or !!value is acceptable.

In the first case, we add a special case for converting an undefined value to true. This is somewhat consistent with other parts of the code base like setFlag(flag) will set the flag to true for a signature setFlag(flag: Flag, value = true).

The current behavior ignores this special case, the existing argument readers correctly handle it with the current logic, and the special case introduces a footgun if a user has a JS config file that returns boolean value as undefined (which normally coerces to false) turning true.

With this in mind, I think it is best left as is, with no special case.

@Gerrit0 Gerrit0 closed this as completed Dec 26, 2019
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

No branches or pull requests

2 participants