You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 unset cert property, but if there is none, perhaps we could have some kind of indication.
@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'm going to remove the good first contribution label 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 if key or cert is 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 key or cert I might suggest then to delegate the throw logic 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.
This 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.
Activity
silverwind commentedon Sep 23, 2015
I agree. We should throw if any of the required options to
{tls,https}.createServerare missing.kulkarniankita commentedon Sep 24, 2015
I agree as well possibly a warning of the property not correctly spelled.
ChALkeR commentedon Sep 24, 2015
@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 commentedon Sep 24, 2015
@ChALkeR Agreed good point. I can work on this if it gets decided to be implemented. (cc: @jasnell )
jasnell commentedon Sep 24, 2015
@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.
kulkarniankita commentedon Sep 24, 2015
I have reproduced the issue, working on adding a fix for 'throwing if any of the required options to {tls,https}.createServer are missing'
silverwind commentedon Sep 25, 2015
👍
https: throw Error if required params missing
Trott commentedon May 3, 2016
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.
flcoder commentedon May 3, 2016
@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.kulkarniankita commentedon May 5, 2016
@Trott I can pick it up as I would like to finish what I started!
Trott commentedon Jul 7, 2017
This 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.