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 support for escaped parentheses in <Route path> #4202

Merged
merged 1 commit into from
Jan 11, 2017
Merged

Add support for escaped parentheses in <Route path> #4202

merged 1 commit into from
Jan 11, 2017

Conversation

sebastiandeutsch
Copy link
Contributor

For our latest project we need to escape parenthesis since they occur in the URLs (which is uncommon but valid). This PR is based on this old one (#2533 by @Zoxive) which has been closed since at some point in time you wanted to refactor the URL matching into a separate module - unfortunately that has not happened yet so it would be very nice if the PR would make it this time.

@timdorr timdorr added this to the v3.0.0 milestone Dec 5, 2016
@timdorr timdorr added the feature label Dec 5, 2016
@sebastiandeutsch
Copy link
Contributor Author

As far as I can see Router v4 can handle escaped parenthesis by delegating the matching mechanism to this module: https://github.com/pillarjs/path-to-regexp

Is it something desirable for v3 as well? I could work on a PR that uses that module instead of having a custom implementation? Are there any barriers that I don't see right now?

@mjackson
Copy link
Member

I believe that using path-to-regexp in v3 may actually break some existing apps that use our custom path syntax. I'm not opposed to merging this into v3. Looks like you've got some decent tests around it too.

@mjackson mjackson changed the title Adding support for escaped parentheses in Route Paths Add support for escaped parentheses in <Route path> Jan 11, 2017
@sebastiandeutsch
Copy link
Contributor Author

Ok didn't know that path-to-regexpwould break stuff. Would be lovely if the PR could make it in the next release. If you need anything - I'm glad to help.

@timdorr
Copy link
Member

timdorr commented Jan 11, 2017

Changing something like that would require a major version bump. And the next major is already reserved 😉

@sebastiandeutsch
Copy link
Contributor Author

Why would the change require a major version bump? Did I change a public API that behaves different now?

@timdorr
Copy link
Member

timdorr commented Jan 11, 2017

No no, I mean if we switched out matchPattern for path-to-regexp in 3.0.

@mjackson
Copy link
Member

I think we can merge this and ship a patch release of 3.0 with it

@timdorr
Copy link
Member

timdorr commented Jan 11, 2017

Yeah, it LGTM too.

@timdorr timdorr merged commit f31a58a into remix-run:master Jan 11, 2017
@timdorr
Copy link
Member

timdorr commented Jan 11, 2017

I'll package this up as 3.0.1 soon. There's some other outstanding bug fixes to push out too.

@sebastiandeutsch
Copy link
Contributor Author

Uhh thanks a lot <3

@sebastiandeutsch sebastiandeutsch deleted the feature/escape-round-brackets branch January 11, 2017 23:11
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants