-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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
tls: re-allow falsey option values #15131
Conversation
5723c4c was an unintentional breaking change in that it changed the behaviour of `tls.createSecureContext()` to throw on false-y input rather than ignoring it. This breaks real-world applications like `npm`. This restores the previous behaviour. Ref: nodejs#15053
Good catch |
LGTM. FWIW the only reason I added those explicit checks was for TurboFan, which prefers more explicit comparisons. |
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.
LGTM
CI run run OSX machine seemed to be offline for last run: https://ci.nodejs.org/job/node-test-pull-request/9947/ |
Is this close to landing? Node master looks like its been unable to run npm for 4 days, it might be better to revert #15053 |
In last ci run: OSX failure -> async-hooks/test-callback-error arm failure is build related. OSX failure
|
Looking at the failing test, does not seem related to tls so I don't think it is related to this failure. Opened #15208 |
It's unrelated, it shows up in all latest builds (caused by the macOS VM rebuilt) |
ok landing change now. |
Landed as 1403d28 |
5723c4c was an unintentional breaking change in that it changed the behaviour of `tls.createSecureContext()` to throw on false-y input rather than ignoring it. This breaks real-world applications like `npm`. This restores the previous behaviour. PR-URL: #15131 Ref: #15053 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: MichaëZasso <targos@protonmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
as the original didn't land I'm markign this dont-land as well please lmk if this is a mistake |
5723c4c was an unintentional breaking change in that it changed the behaviour of `tls.createSecureContext()` to throw on false-y input rather than ignoring it. This breaks real-world applications like `npm`. This restores the previous behaviour. PR-URL: nodejs#15131 Ref: nodejs#15053 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: MichaëZasso <targos@protonmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
5723c4c was an unintentional breaking change in that it changed the behaviour of
tls.createSecureContext()
to throw on false-y input rather than ignoring it. This breaks real-world applications likenpm
.This restores the previous behaviour.
Ref: #15053
/cc @mscdex
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
tls