add undefined type

[leeseean]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

add undefined type

[bnoordhuis]

The commit log should explain what and, importantly, why.

[fhinkel]

@leeseean Thanks so much for your first contribution 🎉 . Looks good to me. Could we change to commit message to something like this?

lib: explicitly check for undefined and null

Instead of checking whether the variable is equal (==) to null, 
check whether the value is (===) null or undefined. Explicit
checks are more precise.

The Contributing.md doc describes how to update a commit message:

$ git commit --amend 
// Make changes to the message and save the file
$ git push --force-with-lease origin master

[apapirovski]

LGTM with the commit message improved

[bnoordhuis]

$ git grep -E ' [!=]= (null|undefined)' lib/ | wc -l
59

That is, we have these checks all over the place. It feels a bit haphazard to update them in just two places.

If I'm being honest, I don't think it's very useful. Replacing == with two === is like saying the same thing but with more words. It's really just restating yourself.

^{(See what I did there?)}

(Wasn't particularly in favour in the first place and since the consensus is against it...)

[BridgeAR]

If anyone is strongly against landing it, please speak up. Otherwise I am going to land this even though I personally agree that in this case there is not much benefit of changing it.

[apapirovski]

I'm pretty -1 on this after having this sit for a while. There's no point to this change given that neither of these values takes an Object (and if someone passes an Object, it throws anyway, making performance irrelevant). The only reason to not use != null is document.all.

I might make it explicit if this is actually being considered to land and a CI gets started.

[BridgeAR]

@apapirovski I meant to land it, otherwise I would not have mentioned it. Your condition makes your "soft" -1 actually a "hard" one because it is not possible to land this in that case. How shall we progress here?

[apapirovski]

@BridgeAR there is no CI at the moment so I don't know if there's intent to land it or not. I just don't see the benefit to this change in this particular part of the code. We have areas where making this change would be beneficial but not here.

Anyway, I won't block it... it's not worth wasting everyone's time with it.

[BridgeAR]

Mini-CI (enough to test this change): https://ci.nodejs.org/job/node-test-commit-light/147/

[BridgeAR]

Landed in 5164a12

@leeseean congratulations on your first commit to Node.js! 🎉

I fixed the commit message while landing.

[MylesBorins]

Do we want to land this on LTS or is it potentially a behavior change?