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

Merged
merged 1 commit into from Jan 11, 2017

Projects

None yet

3 participants

@sebastiandeutsch
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 enhancement label Dec 5, 2016
@sebastiandeutsch
Contributor

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
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 from Adding support for escaped parentheses in Route Paths to Add support for escaped parentheses in <Route path> Jan 11, 2017
@sebastiandeutsch
Contributor

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
Collaborator
timdorr commented Jan 11, 2017

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

@sebastiandeutsch
Contributor

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

@timdorr
Collaborator
timdorr commented Jan 11, 2017

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

@mjackson
Member

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

@timdorr
Collaborator
timdorr commented Jan 11, 2017

Yeah, it LGTM too.

@timdorr timdorr merged commit f31a58a into ReactTraining:master Jan 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@timdorr
Collaborator
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
Contributor

Uhh thanks a lot <3

@sebastiandeutsch sebastiandeutsch deleted the unknown repository branch Jan 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment