Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

engine.io incompatible with restify – engine.io assumes req.query is query hash #203

Closed
timoxley opened this Issue · 9 comments

3 participants

@timoxley

Engine.io assumes if req.query exists that it is an object containing the parsed query:

// transport check
var transport = req.query.transport;
if (!~this.transports.indexOf(transport)) {
  ...
}

Alas, in restify, req.queryis a function.

Request.prototype.query = Request.prototype.getQuery;

This makes engine.io sad as it cannot find its transports:

handling "GET" http request "/result-preview/?EIO=2&transport=polling"
unknown transport "undefined"

Looking for advice on how to solve this issue without hacking modules up.

Perhaps engine.io could also check that .query is an object with the properties engine.io needs, or simply make no assumptions and parse the query string for each request.

Related code in restify:
https://github.com/mcavage/node-restify/blob/9b07786060c240fcb1592289886d21d1f04c99c1/lib/request.js#L178

Related code in engine.io:
https://github.com/LearnBoost/engine.io/blob/605af3a9d952e7e808879405b931de0aabb11085/lib/server.js#L100-L132

@timoxley

FYI I've opened a reciprocal issue on restify: mcavage/node-restify#486

@rauchg
Owner

Awful decision on our side. We shouldn't touch req in this obtrusive way.

@rauchg
Owner

(it was my decision btw :P)

@timoxley

@guille what's the alternative here? engine.io seems to extensively use the req as a namespace for sharing data, you'd need to move to more 'pure' functions or perhaps utilising some kind of wrapper that prevents modification of the original req, e.g. wrapping it in Object.create(req)

@3rd-Eden

maybe all custom properties should be namespaced under req.engineio

@timoxley

@3rd-Eden yep, that's probably the simplest solution. Is there another way though? Although it's the norm, treating req as a dumping ground is a probably a pretty crappy practice in the first place.

@rauchg
Owner

@timoxley ideally we have a helper that just returns the query object from the request, and we don't touch it

@timoxley

@guille ok, req.query is used in multiple locations, but I guess it's probably not so much of a performance hit to just calculate it on the fly.

Also, something similar should probably be done with these other properties attached to the req:

@timoxley

bump

@rauchg rauchg closed this in 0743f32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.