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

Add RegExp path support #68

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add RegExp path support #68

wants to merge 2 commits into from

Conversation

deckerrj
Copy link

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?)

…rve trailing slashes. Support optional express url params.
@joepie91
Copy link
Collaborator

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 joepie91 self-assigned this Mar 17, 2017
@deckerrj
Copy link
Author

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.

@HenningM
Copy link
Owner

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!

@deckerrj
Copy link
Author

deckerrj commented May 2, 2017

@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
Copy link
Author

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

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.

3 participants