Skip to content

Commit

Permalink
Redirect on props update (#5003) (#5162)
Browse files Browse the repository at this point in the history 
* Redirect: Support props changes on Redirect to handle edge cases

* Redirect: Fixed tests after moving to Jest

* Use `createLocation` to parse `to`  to properly handle edge cases
  • Loading branch information
@alexilyaev @timdorr
alexilyaev authored and timdorr committed Aug 23, 2017
1 parent fe16632 commit 2807350
Show file tree
Hide file tree
Showing 2 changed files with 156 additions and 2 deletions.
19 changes: 17 additions & 2 deletions packages/react-router/modules/Redirect.js
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import React from 'react'
import PropTypes from 'prop-types'
import warning from 'warning'
import invariant from 'invariant'
import { createLocation, locationsAreEqual } from 'history'

/**
* The public API for updating the location programatically
* The public API for updating the location programmatically
* with a component.
*/
class Redirect extends React.Component {
Expand All @@ -13,7 +15,7 @@ class Redirect extends React.Component {
to: PropTypes.oneOfType([
PropTypes.string,
PropTypes.object
])
]).isRequired
}

static defaultProps = {
Expand Down Expand Up @@ -49,6 +51,19 @@ class Redirect extends React.Component {
this.perform()
}

componentDidUpdate(prevProps) {
const prevTo = createLocation(prevProps.to)
const nextTo = createLocation(this.props.to)

if (locationsAreEqual(prevTo, nextTo)) {
warning(false, `You tried to redirect to the same route you're currently on: ` +
`"${nextTo.pathname}${nextTo.search}"`)
return
}

this.perform()
}

perform() {
const { history } = this.context.router
const { push, to } = this.props
Expand Down
139 changes: 139 additions & 0 deletions packages/react-router/modules/__tests__/Switch-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,145 @@ describe('A <Switch>', () => {
expect(node.innerHTML).toContain('bub')
})

it('handles subsequent redirects', () => {
const node = document.createElement('div')

ReactDOM.render((
<MemoryRouter initialEntries={[ '/one' ]}>
<Switch>
<Redirect exact from="/one" to="/two"/>
<Redirect exact from="/two" to="/three"/>

<Route path="/three" render={() => <div>three</div>}/>
</Switch>
</MemoryRouter>
), node)

expect(node.innerHTML).toContain('three')
})

it('warns when redirecting to same route, both strings', () => {
const node = document.createElement('div')
let redirected = false
let done = false

spyOn(console, 'error')

ReactDOM.render((
<MemoryRouter initialEntries={[ '/one' ]}>
<Switch>
<Route path="/one" render={() => {
if (done)
return <h1>done</h1>

if (!redirected) {
return <Redirect to="/one"/>
}
done = true

return <Redirect to='/one'/>
}}/>
</Switch>
</MemoryRouter>
), node)

expect(node.innerHTML).not.toContain('done')
expect(console.error.calls.count()).toBe(1)
expect(console.error.calls.argsFor(0)[0]).toMatch(/Warning:.*"\/one"/)
})

it('warns when redirecting to same route, mixed types', () => {
const node = document.createElement('div')
let redirected = false
let done = false

spyOn(console, 'error')

ReactDOM.render((
<MemoryRouter initialEntries={[ '/one' ]}>
<Switch>
<Route path="/one" render={() => {
if (done)
return <h1>done</h1>

if (!redirected) {
redirected = true
return <Redirect to="/one"/>
}
done = true

return <Redirect to={{ pathname: '/one' }}/>
}}/>
</Switch>
</MemoryRouter>
), node)

expect(node.innerHTML).not.toContain('done')
expect(console.error.calls.count()).toBe(1)
expect(console.error.calls.argsFor(0)[0]).toMatch(/Warning:.*"\/one"/)
})

it('warns when redirecting to same route, mixed types, string with query', () => {
const node = document.createElement('div')
let redirected = false
let done = false

spyOn(console, 'error')

ReactDOM.render((
<MemoryRouter initialEntries={[ '/one' ]}>
<Switch>
<Route path="/one" render={() => {
if (done)
return <h1>done</h1>

if (!redirected) {
redirected = true
return <Redirect to="/one?utm=1"/>
}
done = true

return <Redirect to={{ pathname: '/one', search: '?utm=1' }}/>
}}/>
</Switch>
</MemoryRouter>
), node)

expect(node.innerHTML).not.toContain('done')
expect(console.error.calls.count()).toBe(1)
expect(console.error.calls.argsFor(0)[0]).toMatch(/Warning:.*"\/one\?utm=1"/)
})

it('does NOT warn when redirecting to same route with different `search`', () => {
const node = document.createElement('div')
let redirected = false
let done = false

spyOn(console, 'error')

ReactDOM.render((
<MemoryRouter initialEntries={[ '/one' ]}>
<Switch>
<Route path="/one" render={() => {
if (done)
return <h1>done</h1>

if (!redirected) {
redirected = true
return <Redirect to={{ pathname: '/one', search: '?utm=1' }}/>
}
done = true

return <Redirect to={{ pathname: '/one', search: '?utm=2' }}/>
}}/>
</Switch>
</MemoryRouter>
), node)

expect(node.innerHTML).toContain('done')
expect(console.error.calls.count()).toBe(0)
})

it('handles comments', () => {
const node = document.createElement('div')

Expand Down

5 comments on commit 2807350

@whitelizard
Copy link

Choose a reason for hiding this comment

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

Seems that this change can fail if from/referrer is used, it then gets stuck in an infinite recursion. Adding this test would be good.

@timdorr
Copy link
Member

Choose a reason for hiding this comment

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

@whitelizard Can you do a PR?

@pshrmn
Copy link
Contributor

@pshrmn pshrmn commented on 2807350 Aug 25, 2017

Choose a reason for hiding this comment

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

Do you mean something other than these?

<Switch>
  <Redirect from='/dude' to='/sweet' />
  <Redirect from='/sweet' to='/dude' />
</Switch>

or

<Switch>
  <Redirect from '/album:id' to='/album/5' />
</Switch>

Either of those use cases should be on the user to properly handle.

@whitelizard
Copy link

@whitelizard whitelizard commented on 2807350 Aug 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

<Redirect to={{ pathname: '/home', state: { from: currentLocation }, }} />

Failes hard (infinite recursion due to the equal check) if you somehow are on /home. But I guess you mean that the user should prevent this, and I might agree.

We just got this crazy error after the update and traced it to this commit. (Our code could get into a situation of redirecting twice).

@pshrmn
Copy link
Contributor

@pshrmn pshrmn commented on 2807350 Aug 26, 2017

Choose a reason for hiding this comment

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

The locationsAreEqual call should catch when you are attempting to redirect to the same location as the previous redirect. Since that isn't happening for you, there might be a bug, but I would have to see the code that is triggering this. My current guess is that the <Redirect> is mounting on each render for some reason, which is why the update check is not preventing the recursion.

Please sign in to comment.