Skip to content
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

Question about fake .get method #33

Closed
dcousens opened this issue Jun 28, 2016 · 5 comments
Closed

Question about fake .get method #33

dcousens opened this issue Jun 28, 2016 · 5 comments

Comments

@dcousens
Copy link

In index.js, it mentions:

/* By setting this fake .url on the request, we ensure that it will end up in the fake
* .get handler that we defined above - where the wrapper will then unpack the .ws
* property, indicate that the WebSocket has been handled, and call the actual handler. */

What is this fake .get handler?

@EliasHolzmann
Copy link

I'm not the original author, but I had to dive a little bit into this library myself. So, I can't guarantee that what I say is correct, this is only my own possibly flawed understanding ;)

In add-ws-method.js, the fake .get handler is explained quite well. If you say, for example, app.ws("/test", middleware), this calls app.get("/test/.websocket", wrapMiddleware(middleware)) under the hood. This is the "fake .get handler".
If a new request arrives at the server for url /test, and this request is a websocket, the request.url property is also set to /test/.websocket in line 51 of index.js. This does NOT happen for requests which are not meant for websockets, as this all happens in the callback of the ws object (the underlying implementation for websockets, see https://github.com/websockets/ws/). Normal requests keep their request.url property.
The aim of this rewriting is simply to distinguish between websocket requests and normal HTTP requests.

@HenningM
Copy link
Owner

Hi @dcousens,

@bartim's explanation is very good IMO. The "fake get handler" is just a way to get the express router to handle WS requests. The express framework doesn't have a concept of WebSockets, so we let the express router handle the WS upgrade request as a regular GET request instead. This should be OK since these upgrade requests are simple GET requests with an 'Upgrade' header present anyway.

But you'll also notice that the signature of the WS handler functions does not match the signature of the regular GET/POST/PUT+++ handlers as defined by the express framework. This means that we need a way to distinguish our WebSocket upgrade requests from our regular GET requests. That's were the fake handler you're referring to comes into the picture. See @bartim's comment for details on how & where the handler is added.

@dcousens
Copy link
Author

dcousens commented Jul 1, 2016

Thanks guys :)

@xogeny
Copy link

xogeny commented Mar 15, 2017

BTW, I ran into an issue with this. I have routes like /foo/*. This fake handling gets confused in that case because when it redirects /foo/bar/buz to /foo/bar/buz/.websocket, it then matches a different GET route.

My short term solution, which seems to work, is to make sure that the call to app.ws('/foo/*', ...) appears before app.get('/foo/*', ...). In this way, when it redirects, the first thing it considers is the case that it is a websocket.

I'm a little nervous about this because I feel like there will be corner cases where it still might fail (e.g., if I happened to have a route that ended in .websocket).

Ideally, it would be better if the fake URL were rooted in an entirely different URL hierarchy so there was no overlap in the routes. I suppose the applyTo method could be used somehow to achieve that. But for now my simple workaround seems to work so I'm posting that in case anybody else happens across this.

@joepie91
Copy link
Collaborator

joepie91 commented Mar 15, 2017

@xogeny This issue is currently being discussed in #64, but I'm not sure that specifying things in a specific order works around it reliably. The special hierarchy might be an option, I'll have to think about that - but it could cause issues with router support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants