Add notFound function to set respondNotFound callback (#44) #54
Conversation
|
Thanks for the patch. It would be nice also to update the documentation so that this functionality becomes public. |
|
Improved the NotFound function, and updated the readme! :) :) |
| @@ -295,14 +301,14 @@ __Arguments__ | |||
| Gives Redbird a callback function with two parameters, the ExpressJS request | |||
There was a problem hiding this comment.
Its not a expressJs object but just the standard http request and response objects: https://nodejs.org/api/http.html#http_class_http_clientrequest https://nodejs.org/api/http.html#http_class_http_serverresponse
| @@ -174,7 +174,7 @@ function ReverseProxy(opts){ | |||
| if(target){ | |||
| proxy.ws(req, socket, head, {target: target}); | |||
| }else{ | |||
| notFound(socket); | |||
| respondNotFound(req, socket); | |||
There was a problem hiding this comment.
This isn't an HTTP response object, but does a socket have the same set of properties and methods as an HTTP response?
There was a problem hiding this comment.
it has the write and the end, I am not sure of the status method, probably not. I think that this codepath never happens in practice though, thats why it has been working so far.
There was a problem hiding this comment.
I'm not sure if it would be valid anyway to attempt to send a "not found" over a socket.
Actually, wouldn't the destination already have had to be found before the server can emit the "upgrade" event? So there would be no need for the custom error message because that should never happen in practice (unless you have the proxy settings changing in the middle of a connection or something weird like that)
There was a problem hiding this comment.
thats what I meant in my previous comment :)
There was a problem hiding this comment.
:P
If we don't seem to really care about this use case, we can perhaps open a separate issue and merge this? I haven't actually tested whether sockets work at all, so I don't know haha
This should fix #44. Perhaps we should add a test for this, too.