Force strict comparison

[thefourtheye]

This patch replaces all the non-strict comparisons to strict comparisons.

CI Run: node-test-pull-request/94

[thefourtheye]

CI is Green (the only failing test's failure is not because of this change)

[mscdex]

+1

[ronkorving]

Better review and merge this asap to avoid lots of merge conflicts in the future.

[ronkorving]

I would also immediately update lint options to enforce this, or it will slowly trickle back into the codebase.

[thefourtheye]

cc @nodejs/collaborators

[domenic]

This seems extremely likely to cause regressions. Just glancing over the code, I see a lot of cases where input that would previously be accepted (e.g. "0") is no longer accepted (because == 0 changed to === "0"). While I haven't confirmed that this is actually a problem in practice, I'm very uncomfortable with this. If we do this, I think we should expect some programs to break in very subtle ways, and for the next few releases to contain a bunch of minor fixes (e.g. changing x === 0 to +x === 0) as the regressions are discovered.

[reqshark]

perhaps @domenic is right and the PR is overzealous; therefore, each case should be looked at carefully.

At any rate, i think this effort is moving the codebase in the right direction!

@thefourtheye nice PR! I'm in favor. :) +1

[thefourtheye]

@domenic Most places where the the value is compared against zero, its the length of something. I think we should be fine there. Shall I do the +x === changes now itself?

@reqshark Thanks :-) Please feel free to review and leave suggestions when you find time :-)

[domenic]

I don't think any changes should be done in an automated way. I think every single change needs to be reviewed extremely carefully.

[thefourtheye]

I agree, I would be doing the changes manually only.

[Trott]

Agree 100% with @domenic regarding risk of subtle bugs. I'd definitely want to see this run through whatever smoke testing is available at this point.

While I favor the "always use strict equals" style, I'm not convinced retrofitting it here is the way to go. In addition to the subtle bugs risk, a couple other concerns:

• This makes some of the code more difficult to understand. The change to lib/cluster.js takes a relatively simple line of code and makes it something significantly more convoluted.
• This seems likely to introduce dead code in conditionals. For example, in lib/cluster.js again, is it even possible for process.exitCode to be null in some cases and undefined in others? I haven't looked, so maybe it is, but that's the sort of thing I'm referring to.

I would prefer this sort of thing be split into multiple PRs with related changes grouped together so that they can be more easily and confidently reviewed. I acknowledge that it's more tedious for the submitter and time-consuming that way.

[ronkorving]

So close this and chunk it up? Makes sense to everyone I guess.

[thefourtheye]

Closing this, as the change could lead to unexpected consequences.

[thefourtheye]

Based on the comments, I submitted another patch at #2582, which simply uses strict operators with string comparison. PTAL.