React-router: Redirect component kills infinite redirect loop (good thing) which masks the issue (bad thing)

[vancityhuddy]

Version

4.0.0

Test Case

https://codepen.io/anon/pen/dWGwwY

Steps to reproduce

Code pen example has this issue shown when visiting About page twice!. Click About > Home > About.

Have some logic in your app that redirects the page if a certain parameter isn't set, or set wrong. Upon visiting the page a second time you might step into that redirect unintentionally. This is a simple logic error caused by the developer, fair enough. However, the Redirect component seems to swallow this without notifying the developer, causing great pain and anguish in trying to track down how a page is now blank

Expected Behavior

Maybe a console log in ENV=development that 'Redirect did not cause a change to URL' or something like that?

Actual Behavior

Pain.
Anguish.
(both great)

[pshrmn]

Alright, just for anyone else that reads this, this is the issue:

1. A <Route> matches a location and renders.
2. One of the child components that gets rendered is a <Redirect>.
3. That <Redirect> redirects back to the same route.
4. When the <Route> re-renders, instead of mounting the<Redirect> just updates.
5. There is no update behavior for the <Redirect> because it is assumed that a <Redirect> should never actually update.

What this means is that if the <Redirect> is the only thing that is rendered, you will end up with a blank screen.

I'm not really sure that a fix is necessary, but _if_ we were to address it, the easiest approach would probably be to add a componentDidUpdate method to <Redirect> that calls warning.

componentDidUpdate() {
  warning(false, 'A <Redirect> should never update.')
}

[Dustin-RW]

Hello, could I take this on as a beginner commit?

[timdorr]

No need to ask, just submit a PR 😄

[alexilyaev]

@Dustin-RW @pshrmn The case of redirecting to the same route should definitely warn, blank pages are a frustrating dev experience (just had it and spent 2 hours getting to a similar conclusion as @pshrmn before reading this issue).

But... there's another case...
https://www.webpackbin.com/bins/-KjU_CGVVfWDb1M3jCXV

See Root.js:

function Root() {
  console.log(window.location.pathname);

  return (
    <Switch>
      <Redirect exact from="/" to="/hello" />
      <Redirect exact from="/hello" to="/hello/world" />

      <Route exact path="/hello/world" component={ Hello } />
    </Switch>
  )
}

The redirects will actually change the URL, but since Switch creates a clone of the child (Redirect in this case), and the previous child was also a Redirect, there's no logic on props change, and Redirect renders null, that's why we get a blank area.

So in Redirect.js, we can add a componentWillReceiveProps and:

1. If the props are the same, warn as @pshrmn mentioned
2. If the props are not the same, callthis.perform()

Questions:

• Does this.isStatic play a role in this case?
• componentWillReceiveProps vs. componentDidUpdate, which would be better in this case?

@Dustin-RW Are you on it, or do you want me to handle it?

[alexilyaev]

@pshrmn I'd like to take this, can you answer my questions in the comment above?

[pshrmn]

1. isStatic should not matter here. isStatic is only used for server rendering, where there are no post-render lifecycle events. Since this is an issue with multiple renders, it should be client-side only.

2. componentDidUpdate would be better. This is because it mirrors componentDidMount, and it doesn't require a perform rewrite (you would have to pass nextProps/nextContext if done from cWRP in order to redirect using the new props).

As a temporary fix, you could set a key on your <Redirect>s so that you are creating a new <Redirect> instead of re-using the existing one.

<Switch>
  <Redirect
    exact
    from="/"
    to="/hello"
    key="from-root"
  />
  <Redirect
    exact
    from="/hello"
    to="/hello/world"
    key="from-hello"
  />
  <Route exact path="/hello/world" component={ Hello } />
</Switch>

[alexilyaev]

@pshrmn Cool, thanks.
key worked, btw.
Opened a PR.

[wsfuller]

Is there any movement on this PR? It looks to have been hanging for almost 3 weeks. Having an issue #5231 and hoping that this PR with using Keys will be able to solve the problem. Having extreme difficulties getting clarity from multiple channels of communication so this would be nice to try.

[alexilyaev]

@wsfuller Done https://github.com/ReactTraining/react-router/pull/5162#issuecomment-308269859 :-)

Note that adding key did work as it is today.
The PR addresses both subsequent redirects and redirecting to the same route you're currently on.

[wsfuller]

@alexilyaev Thank you so much! Been really struggling with this problem for the past several days pulling out hair