-
Notifications
You must be signed in to change notification settings - Fork 30k
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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: MichaëZasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Brian White <[email protected]>
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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: MichaëZasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Brian White <[email protected]>
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