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

use _query instead of query, closes #203 #241

Merged
merged 1 commit into from
Apr 9, 2014
Merged

Conversation

yocontra
Copy link

@yocontra yocontra commented Apr 4, 2014

fixes compat with restify and all other things that already use the req.query attribute

@yocontra
Copy link
Author

yocontra commented Apr 4, 2014

Related: #203

@rauchg
Copy link
Contributor

rauchg commented Apr 4, 2014

The requests that engine.io captures shouldn't have been touched by restify, right? So why would there be a collision ?

@yocontra
Copy link
Author

yocontra commented Apr 4, 2014

@guille

restify overrides the http IncomingMessage prototype and tacks on a bunch of stuff

Specifically this https://github.com/mcavage/node-restify/blob/master/lib/request.js#L176 was causing the breakage.

Unrelated but this was also an issue: https://github.com/mcavage/node-restify/blob/master/lib/server.js#L250

It seems like they manually whitelisted socket.io to ignore from their stuff. Polling requests for engine.io were still ending up in their routing system where it was adding headers and generally being shitty. I was able to get around this by binding directly to the internal http server within restify. Open issue is at restify/node-restify#575

@rauchg
Copy link
Contributor

rauchg commented Apr 9, 2014

I don't mind having req._query.

rauchg added a commit that referenced this pull request Apr 9, 2014
use _query instead of query, closes #203
@rauchg rauchg merged commit 0743f32 into socketio:master Apr 9, 2014
darrachequesne pushed a commit that referenced this pull request May 8, 2020
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

Successfully merging this pull request may close these issues.

2 participants