Repository navigation
Possibly Trivial HTTPS Options Issue #3024
Description
Activity
I agree. We should throw if any of the required options to
{tls,https}.createServerare missing.I agree as well possibly a warning of the property not correctly spelled.
@kulkarniankita That won't happen. Check if it is missing is fast and is a good thing to do (when the property is required), and trying to find a property with a similar name would require excess code and time, and is a no-go.
@kulkarniankita ... +1 ... I agree with @silverwind, throwing if the required options are missing is a good idea. The only concern is that adding new throws is a semver-major but that's easily managed.
I have reproduced the issue, working on adding a fix for 'throwing if any of the required options to {tls,https}.createServer are missing'
👍
- added a commit that references this issue
on Oct 24, 2015 I'm going to remove the
good first contributionlabel as this is a fair bit more complicated than it appears at first blush. For starters, there are ciphers that do not require key/cert, so merely throwing an error ifkeyorcertis missing is not correct.It appears that there is a good start on this at #3064 but that PR got stalled. Pinging @kulkarniankita to see if maybe she's ready to pick it back up and finish it off, because that would be awesome.
@Trott In the case of possible ciphers not using
keyorcertI might suggest then to delegate thethrowlogic to whatever consumers(ciphers in this case) are dependent upon the option. Ciphers can designate what options they require and the initial options handler can then validate the options. In any case, I feel the rule of thumb is this: if a consumer of options requires a number of those options to function properly, those required options are no longer optional. They may start out as options when provided to the first handler, but once the handler delivers them to the end-consumer (cipher), they could turn into required arguments. My final input is this... if it is a case where performance were an issue, like inside a loop, such as a rendering loop, then of course you're not going to be checking properties and doing validations just to help out the coder, but within a process that is generally within initialization steps and only being done once or a few times, I think, even if a bit of refactoring is required, it's definitely worth X minutes of lib dev's time and Y nanoseconds of options validation time in order to save Z (minutes/hours/days x number of lib consuming devs) time. In the end, no one will thank you... because when the exception is thrown, they'll fix their code and move on without ever realizing that some lib dev saved them a lot of time, but I guess the upside is that no one will growl at you or worse, lose faith in your lib. I should probably go take an anti OCD pill now, but I'll end with saying, good work overall, node is awesome and has changed my life.@Trott I can pick it up as I would like to finish what I started!
Reacted by Rich TrottThis issue has been inactive for sufficiently long that it seems like perhaps it should be closed. Feel free to re-open (or leave a comment requesting that it be re-opened) if you disagree. I'm just tidying up and not acting on a super-strong opinion or anything like that.
This is more of a suggestion than an issue. So, instead of having
{cert: ***}, I had{crt: ***}, and there was no indication of anything amiss; the server started listening. Chrome was giving me ERR_SSL_VERSION_OR_CIPHER_MISMATCH. I googled my ass off and tried various things. There may be a reason to allow the options to have an unsetcertproperty, but if there is none, perhaps we could have some kind of indication.