-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
lib: change abstract equal to strict equal #22974
Conversation
@@ -349,7 +349,7 @@ Console.prototype.table = function(tabularData, properties) { | |||
if (properties !== undefined && !ArrayIsArray(properties)) | |||
throw new ERR_INVALID_ARG_TYPE('properties', 'Array', properties); | |||
|
|||
if (tabularData == null || typeof tabularData !== 'object') | |||
if (tabularData === null || typeof tabularData !== 'object') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not obvious, but this significantly changes the semantics of the check in a non-backwards compatible way.
null == null; // true
undefined == null; // true
null === null; // true
undefined === null; // false
The current check makes sure that tabularData
is also not undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the later one can check it, typeof undefined === 'undefined'
so typeof tabularData !== 'object'
is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is 100 % subjective, but maybe this would be a bit more Obviously Correct™ if the conditions were switched?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just not seeing the point of the change then if it simply shifts the undefined
case a bit to the right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just change to strict equal to unify the overall code style.
If there is any problem, feel free to close it.😜
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, I'm not overly happy with it but I won't block it :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment.
Resumed build: https://ci.nodejs.org/job/node-test-pull-request/17424/ |
PR-URL: #22974 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
PR-URL: #22974 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Thanks all of you. |
PR-URL: #22974 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
PR-URL: #22974 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: John-David Dalton <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
Just change the rare
==
to===
, nothing else be affected.Unify the style, may looks more comfortable 😄.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes