Conversation
|
So this would only fail for people using verbs that are outside of any http spec? That doesn't look like a breaking change in my book. Could that be a problem for people using unix sockets maybe? |
Not exactly. The HTTP spec allows any combination of letters with case sensitivity. However, the node http parser only supports a limited subset (as listed in Http.METHODS), and any outside this will error. As such, any routes with these methods will be unreachable. |
|
Whether unix sockets are used is irrelevant, as this relates to the content of the stream, not how it is made. |
Nargonath
left a comment
There was a problem hiding this comment.
I understand that it could prevent a server from starting with this new release hence the breaking change. It targets code that would never be reachable anyway so I'd be tempted to stamp it as bugfix. However if we take the strict definition of breaking change which is, IMO, a release that breaks users flow without any change on their side it should be flagged as BC. It's tricky though since most people actually have proper HTTP verbs if they want their code to work so I'd expect the impact to be low.
Use the node.js built-in parseable http method list to reject routes that can never be reached.
Given that this will make hapi fail to start on existing code that inadvertently uses an unsupported method (maybe a typo), this might be more suited for including in a breaking release.