HenningM / express-ws

WebSocket endpoints for express applications
BSD 2-Clause "Simplified" License
877 stars 142 forks source link

Unable to use RegExp object to define routes #52

Open john-doherty opened 7 years ago

john-doherty commented 7 years ago

I'm trying to use:

router.ws(new RegExp('^/([0-9]{6})$'), function (ws, req) {
// do stuff
});

But it's throwing an error on line 18 of websocket-url.js because it's always assuming a string:

/* The following fixes HenningM/express-ws#17, correctly. */
function websocketUrl(url) {
  if (url.indexOf('?') !== -1) {
    var _url$split = url.split('?');

    var _url$split2 = _slicedToArray(_url$split, 2);

    var baseUrl = _url$split2[0];
    var query = _url$split2[1];

    return (0, _trailingSlash2.default)(baseUrl) + '.websocket?' + query;
  }
  return (0, _trailingSlash2.default)(url) + '.websocket';
}
joepie91 commented 7 years ago

Hmm, this is actually a non-trivial thing to fix.

The problem is that to integrate with Express, express-ws will create a 'fake' suffixed GET route that applies all the specified middleware, before actually doing anything with the WebSocket. To create this fake route, it must add a suffix like .websocket at the end, to make sure it doesn't conflict with existing routes.

However, in the case of a regular expression, you can't just "add a string at the end". While it might be possible to implement this, it would involve a very carefully written regex-aware implementation of the suffixing code.

This is complicated by the fact that Express has multiple route formats, not all of which are documented, including a string-based regex-containing one.

I might take a crack at this in the future when I can find the time, but for now the easiest workaround would be to just use string-based route parameters, and handle 'validation' of the parameters within the route itself.

john-doherty commented 7 years ago

I agree @joepie91 and that's what I've done for now. I did consider updating the code and issuing a pull request, but like you, I realized it was going to be quite a bit of work and require a good few unit tests.

Likewise, if I find some free time I'll take a look at it - I'll let you know first of course.