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

Support <Redirect> in <Switch> #4546

Merged
merged 1 commit into from
Feb 17, 2017
Merged

Conversation

wmertens
Copy link
Contributor

  • <Redirect> already worked because it doesn't have a path and it gets rendered :)
  • Add from as an alias for path in matchPath so you can write <Redirect from="/old" to="/new"/>

Fixes #4541

* `<Redirect>` already worked because it doesn't have a `path` and it gets rendered :)
* Add `from` as an alias for `path` in `matchPath` so you can write `<Redirect from="/old" to="/new"/>`

Fixes remix-run#4541
@@ -30,7 +30,8 @@ const matchPath = (pathname, options = {}) => {
if (typeof options === 'string')
options = { path: options }

const { path, exact = false, strict = false } = options
const { exact = false, strict = false } = options
const path = options.path || options.from
Copy link
Member

Choose a reason for hiding this comment

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

Oh wow, that wasn't a huge change 😅

Copy link
Member

@ryanflorence ryanflorence Feb 21, 2017

Choose a reason for hiding this comment

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

Seems out of place to me. The components define this API (from/to/path) not matchPath. Then this opens up weird stuff like <Route from/> doesn't it?

Copy link
Member

Choose a reason for hiding this comment

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

Also this whole API is a little weird to me. What does <Redirect from="..." to="..."> mean outside of a <Switch>?

Copy link
Member

@ryanflorence ryanflorence Feb 21, 2017

Choose a reason for hiding this comment

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

I have fewer questions with this:

const RedirectFrom = ({ from, to }) => (
  <Route path={from} render={() => <Redirect to={to}/>}/>
)

Copy link
Member

Choose a reason for hiding this comment

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

Then this opens up weird stuff like doesn't it?

Who cares? It's an implementation detail. It's private API, we don't document it, and if anyone does any thing like <Route from>, we don't support it.

Copy link
Member

Choose a reason for hiding this comment

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

What is <Redirect from to/> intended to do outside of <Switch/>?

Copy link
Member

Choose a reason for hiding this comment

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

Nothing. I guess we could create a new component for the use case of <Redirect> inside a <Switch>, but we could also just tell people "don't do that".

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine. <Switch> is kind of magical anyway. It reads the data of its children and decides what to do based on their props. We can pretty easily say <Redirect from> is only supported inside a <Switch>.

Copy link
Member

Choose a reason for hiding this comment

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

It could also just hang out and wait for that from path to match and redirect when it does ... hmm...

@mjackson mjackson merged commit e5cda00 into remix-run:v4 Feb 17, 2017
@mjackson
Copy link
Member

Thanks, @wmertens!

@wmertens wmertens deleted the switchRedirect branch February 18, 2017 18:24
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 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

Successfully merging this pull request may close these issues.

3 participants