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

Added Redirect with parameters #5209

Merged

Conversation

dlindenkreuz
Copy link
Contributor

@dlindenkreuz dlindenkreuz commented Jun 5, 2017

This pull request adds back a feature from v3 where the to URL pathnames of <Redirect> could contain parameters.

Currently, parameters in from are already matched by <Switch> and then passed into <Redirect> as computedMatch prop just like <Route> children. However, <Redirect> does not do anything with that information yet.

I implemented a new generatePath(pattern, params) function that creates a new pathname using path-to-regexp's compile function. This guarantees that patterns are processed in the same way as they are parsed in matchPath. This function is used to put the params from computedMatch into the respective slots of the to prop, which may from now include named parameters as well.

All compile functions are cached by pattern using the same mechanism as matchPath.

Most importantly, the matching logic remains completely untouched and as-is. All of this is meant to be minimally invasive.

Tests, docs and batteries included. It might make sense to make generatePath available as a public API helper method and document it like matchPath.

Examples (without <Switch> for brevity)

// /users/123 → /users/profile/123
<Redirect from="/users/:id" to="/users/profile/:id" />

// /users/123 → /users/profile/123?additional=args
// Location objects work as well, `to.pathname` gets populated with parameters
<Redirect from="/users/:id" to={{ pathname: "/users/profile/:id", search: "?additional=args" }} />

// /users/editors/123 → /users/profile/123
// parameters in `from` that are not included in `to` are ignored
<Redirect from="/users/:group/:id" to="/users/profile/:id" />

// /users/editors/123 → /users/home
// `to` can be completely "static"
<Redirect from="/users/:group/:id" to="/users/home" />

// /foo/bar → does not match, no redirect (duh)
<Redirect from="/users/:group/:id" to="/users/home" />

// /users/123 → ERROR
// Parameter :foo is missing in `from`, path-to-regexp throws an error
<Redirect from="/users/:id" to="/users/:id/:foo" />

// /users/profiles → /profiles
// Needless to say, all redirects without parameters work just like usual.
<Redirect from="/users/profiles" to="/profiles" />

*/
const generatePath = (pattern = '/', params = {}) => {
if (pattern === '/') {
return pattern
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously, this pattern does not contain any parameters. For an additional performance shortcut, we could assume that all strings that do not contain *, (, ) or : are eligible to pass through before creating and caching a compile function.

@MichalZalecki
Copy link

Is there anything which keeps this PR from being merged?

@amaschas
Copy link

amaschas commented Aug 9, 2017

I was actually just about to file a request for this feature. Seems like it's been waiting a while for a merge.

@mjackson mjackson self-assigned this Sep 18, 2017
@jochenberger
Copy link
Contributor

Just out of curiosity, do you see any solutions for e.g. /users/123 → /users?id=123 or should that better be done in a programmatic way?

@mjackson
Copy link
Member

Thank you @dlindenkreuz for implementing this! I've been wanting to tackle this for a while now. This will definitely make it into our next release 😅

@dlindenkreuz
Copy link
Contributor Author

@mjackson Sweeeet 🎉

@jochenberger path-to-regexp, the underlying library that is responsible for matching paths and extracting URL parameters, does not handle query strings at all.

More details:

You might want to wrap the redirect in a <Route> like this (not tested, might not work exactly as-is):

const dynamicRedirect = ({ match }) =>
  <Redirect to={{pathname: "/users", search: `?id=${match.params.userId}`}} />
// ...
<Route path="/users/:userId" render={dynamicRedirect} />

@lionkeng
Copy link

lionkeng commented Dec 7, 2017

Will there be a release anytime soon that includes this change? Thx.

@zakness
Copy link
Contributor

zakness commented Jan 9, 2018

This implementation does not account for paths with param modifiers. For example:

<Redirect from="/about/:rest+" to="/:rest+" />
// expected: /about/foo/bar → /foo/bar
// actual: /about/foo/bar → /foo%2Fbar

I did a quick fix in my own project by just appending .replace('%2F', '/') to the output of generatePath, but this needs some more thinking + additional tests. I may have time to contribute something soon, just wanted to raise it here.

@barrystaes
Copy link

barrystaes commented Jan 17, 2018

Will there be a release anytime soon that includes this change? Thx.

Its not in the currently latest release of react-router-dom 4.2.2
(..there really should be better way to look up in which version a commit got released!)

@timdorr
Copy link
Member

timdorr commented Jan 17, 2018

@barrystaes There is. Or look up the tags.

@barrystaes
Copy link

Thanks @timdorr!

Not sure on how to find it (by commit) in Github however.. like git describe --exact-match <commit-id> or git tag --contains <commit>..

@timdorr
Copy link
Member

timdorr commented Jan 17, 2018

screen shot 2018-01-17 at 10 02 33 am

@liuyenwei
Copy link

👋 bumping this since I would also love this change. are there any plans for a release?

@timdorr
Copy link
Member

timdorr commented Feb 8, 2018

Yes, it will be released.

@imevro
Copy link

imevro commented Feb 21, 2018

@timdorr do you have ETA?

@timdorr
Copy link
Member

timdorr commented Feb 21, 2018

Nope.

@Xuhao
Copy link

Xuhao commented Feb 28, 2018

Looking forward to the release.

@klairetan
Copy link

Also looking forward to this release!

@lock lock bot locked as resolved and limited conversation to collaborators Jun 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet