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

react-router-config matchRoutes matches incorrectly when using query params #5132

Closed
michaelBenin opened this issue May 17, 2017 · 6 comments

Comments

@michaelBenin
Copy link

Version

1.0.0-beta.3

Test Case

Test case not available as unpkg is broken see this issue: #5130

Steps to reproduce

Example:

var matchRoutes = require('react-router-config').matchRoutes;

function noop() {}

var routes = [
  {
    path: '/search/:q',
    component: null,
    loadData: noop
  },
  {
    path: '/search',
    component: null,
    loadData: noop
  },
  {
    path: '/:search(/search?q=/)',
    component: null,
    loadData: noop
  },
];

var branch = matchRoutes(routes, '/search?q=thequery');

console.log(branch); // [] matches nothing

Expected Behavior

Matches /search

Actual Behavior

Matches nothing.

@nowy
Copy link

nowy commented May 17, 2017

@michaelBenin are you creating a PR for this? would be interested in giving this a go if not

@timdorr
Copy link
Member

timdorr commented May 17, 2017

There currently isn't a way to match the search part of the URL. We use path-to-regexp and it has the following limitation:

Please note: The RegExp returned by path-to-regexp is intended for use with pathnames or hostnames. It can not handle the query strings or fragments of a URL.

So, this isn't something we can support on our end. In addition, we don't process the query string at all anymore, so there isn't really a good way to handle this yet.

This can likely be solved with something like #4962, where you could set up a condition property on each route that has a function which is executed to determine if the route should match or not.

Since it's heavily related to that, I'll close this out in favor of that issue. A PR for it would be awesome!

@timdorr timdorr closed this as completed May 17, 2017
@michaelBenin
Copy link
Author

That's how we got around it by using the path instead of the entire url.

@timdorr
Copy link
Member

timdorr commented May 17, 2017

But it's dependent on query variable order at that point, so it's brittle behavior. Instead, you should pass off the location to a function that can parse the query string and do whatever logic you want, including checking against external resources.

Bonus points for a condition function that accepts Promises!

@michaelBenin
Copy link
Author

michaelBenin commented May 17, 2017

Tomorrow morning we are releasing the new https://www.fastcompany.com/ all converted to react-router v4.

On this page: https://github.com/ReactTraining/react-router/tree/master/packages/react-router-config

Where it states:

This is alpha software, it needs:
Realistic server rendering example with data preloading
Pending navigation example

I'd be happy to share how we were able to migrate from v3 to v4 replacing onEnter with using history.listen. We currently are using all 4 packages this repository provides. Thank you again for all the great work. We were able to upgrade a fairly large complex server rendered app painlessly, (I say this now, prelaunch.)

@michaelBenin
Copy link
Author

@nowy I'm not creating a pr. My team is using the path instead of full url. Thank you.

@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
None yet
Projects
None yet
Development

No branches or pull requests

3 participants