express-ws icon indicating copy to clipboard operation
express-ws copied to clipboard

Add RegExp path support

Open deckerrj opened this issue 8 years ago • 5 comments

Added support for modifying RegExp paths. Also fixed a few bugs, including: properly handling hash url elements, preserving trailing slashes in URLs, and supporting optional express url params (ala :param?)

deckerrj avatar Mar 16 '17 17:03 deckerrj

Thanks for your PR! I do have a few concerns, however:

  1. Did you verify that it doesn't break router support? You've changed .websocket from a suffix to a prefix, but it being a suffix is precisely because router support would otherwise break.
  2. You've added support for "hash URL elements", but a fragment (the part after a #) should never be sent to the server (over HTTP) in the first place. What issue is this meant to fix?
  3. A similar question for "preserving trailing slashes in URLs" - what problem is this meant to fix? I don't see any open issues about this.

joepie91 avatar Mar 17 '17 19:03 joepie91

I retained the existing .websocket suffix, it has not changed to a prefix - if you see otherwise, I'd appreciate if you could point this out in the code. The hash change was a simple enough modification of the regex - I have some apps that route URLs internally, and wanted to make sure that both ? and # would be properly supported. The trailing slash preservation is due to the express "strict" routing option, that differentiates URLs that end with slash from non-slash. It seemed prudent to add support for that particular case.

deckerrj avatar Mar 18 '17 23:03 deckerrj

Hi @deckerrj,

First of all: this looks like a solid pull request. Neat & concise solution to what I assumed would require a lot more fiddling and ugly hacks. I haven't yet tested all the possible edge cases that have to be covered here, but so far it seems to work quite well.

I am also a bit curious about point #2 from @joepie91's list though. Since this library is intended to be used on the server side I don't see any reason to include hash fragments as part of the URL RE. So if there's a legitimate reason for doing this then please let us know.

Thanks for the contribution!

HenningM avatar Mar 31 '17 12:03 HenningM

@HenningM You bring up a fair point about the server side parsing. Since the hash element doesn't crop up in my internal routing I've added another commit to remove it. Let me know if you want me to squash the two together.

deckerrj avatar May 02 '17 19:05 deckerrj

@HenningM @joepie91 Is there anything else you need from me before merging this?

deckerrj avatar Feb 15 '18 17:02 deckerrj