React-router: Add <Redirect push> and <Link replace> to override defaults

[aaugustin]

Here's some feedback about the <Redirect> API in v4. There's a few suggestions at the bottom.

I started to use <Redirect> to redirect to a new URL after a successful form submission with the following pattern (simplified here, I'm just keeping the bits related to <Redirect>):

class MyForm extends React.Component {

    state = {
        result: {}
    }

    handleSubmit() {
        // ...
        if (result.isValid) {
            this.setState({result})
        }
    }

    render() {
        const {result} = this.state
        if (result.isValid) {
            return <Redirect to="/success/" />
        }
    }

}

Basically this is a client-side implementation of the old server-side POST-redirect-GET pattern.

I noticed that with this implementation the Back button doesn't work. Indeed <Redirect> doesn't add a new entry to the history.

The docs say:

Rendering a Redirect will navigate to a new location and add the previous location onto the next location state.

I'm failing to understand what "adding onto the next location state" means. I guessed it meant "adding an entry to the history", but apparently, it's rather "overwriting the current location with the next location". Unless this sentence talks about something else entirely?

Here's a few some suggestions for which I'm happy to provide a pull request if you think they're appropriate.

1. make transitionTo the default behavior of <Redirect>, as it's less likely to corrupt history inadvertently than replaceWith. By defaulting to replaceWith, the seemingly innocuous use of <Redirect> I showed above doesn't record history, which is a bug. I'm afraid many websites could end up with broken histories because of this.
2. make it possible to redirect either with transitionTo or replaceWith, perhaps by providing one component for each behavior, or a prop on Redirect to select the behavior. I'm unsure when replaceWith would be appropriate, but I assume you had a reason to choose it and would like to preserve that possibility in the API.
3. if appropriate, clarify that sentence I quoted above — I'm not a native speaker and I admit that I can be clueless when it comes to frontend, but I've been developing for the web for 10 years and I'm a Django core dev, so not a total noob; I've also written several dozen pages of docs for Django; if I don't get something, it's likely that I'm not the only one.

Thanks for your consideration! Let me know if some of these ideas seem worth discussing further.

[timdorr]

We probably want to add a prop to <Redirect> to switch between what kind of transition it does. <Redirect replace={true} />.

A PR to add this would be great!

[ryanflorence]

I've been thinking Redirect and Link could both use an "action" prop:

<Redirect to={to} action="PUSH"/>
<Link to={to} action="REPLACE"/>

Or they get their own special API, opposite of their default

<Redirect to={to} push/>
<Link to={to} replace/>

[timdorr]

I like the boolean option the most. No chance to fat finger it or try to put in weird values and get upset when it doesn't work.

[aaugustin]

Good, let's go with a boolean option and add it to <Link> as well.

I would like to explain in the documentation why <Redirect> defaults to replacing the current URL then demonstrate <Redirect push />.

However I'm still unsure of the reason for that default; it seems to me that a visible change in the URL should be undoable by pressing "Back" in general.

Is <Redirect> originally designed for "internal router redirects" rather than "redirect in response to a user action"?

[fhelwanger]

I also expected <Redirect> to push instead of replace by default. In my apps, push is much more common than replace.

In this page from the docs it says:

(If this freaks you out you can use the imperative API from the router on context.)

If I can use context, shouldn't something like withRouter be provided?

[aaugustin]

As far as I understand, using the context means:

const {router} = this.context
router.transitionTo(...)

[fhelwanger]

Yes, I know I could do this.

But it isn't a good practice to let users access this.context directly, as noted here.

We agree that hiding context usage from consuming components is a good idea until the context API stabilizes. However, we recommend using higher-order components instead of mixins for this.
...
If you’re using a third party library that only provides a mixin, we encourage you to file an issue with them linking to this post so that they can provide a higher-order component instead. In the meantime, you can create a higher-order component around it yourself in exactly the same way.

Providing some HOC such as withRouter, besides requiring users to write less boilerplate do access context, is more secure to future changes of the context API.

In fact, it was already done in v2: https://github.com/ReactTraining/react-router/issues/3350

[aaugustin]

Ah, sorry. As far as I can tell, this can be dealt with separately, it may be best to discuss that in a separate issue?

[aaugustin]

3912 implements what @ryanflorence and @timdorr described above.

I wrote integration tests because <Redirect> doesn't do anything when it isn't wrapped in a <Router>. The tests are a bit verbose because I chose to inject callbacks rather that introduce a mocking library — currently the project doesn't use one.

I would prefer to make transitionTo the default behavior in all cases and provide <Link push /> and <Redirect push /> for developers who want the alternative behavior, but since that part of the proposal wasn't discussed yet, I haven't implemented it. I'll adjust the PR in that direction if it's accepted.

[mjackson]

However I'm still unsure of the reason for that default; it seems to me that a visible change in the URL should be undoable by pressing "Back" in general.

The default behavior of <Redirect> is modeled after server-side redirects (i.e. HTTP status 301 or 302) which do not add an entry to the browser's history. Since this is what most people think of when we say the word "redirect" we imitate this behavior so they aren't surprised.

You can test this behavior by going to https://unpkg.com and clicking on any un-versioned link (e.g. https://unpkg.com/react). This will issue a 302 redirect and take you to a URL that includes the latest React version (https://unpkg.com/[email protected] as of this writing). When you click your back button, instead of being taken to https://unpkg.com/react you'll be taken back to https://unpkg.com because your browser didn't store an entry for https://unpkg.com/react in the history stack (it was a redirect!).

Feel free to use that explanation in the docs if it makes sense to you :)

[aaugustin]

Thanks a lot for that explanation.

The parallel works in your example.

I tried writing it down for my use case and eventually found out how it's supposed to work.

_server-side app_

User clicks <a href="/form">.

1. GET /form => 200 - create history entry 1 - display form
2. POST /form => 302 - no new history entry - redirect to /done
3. GET /done => 200 - new history entry 2 - display confirmation

_client-side app_

User clicks <Link to="/form">.

1. pushState /form => - create history entry 1 - display form
2. submit form with fetch - _must pushState history entry 2 here_ - render <Redirect to="/done">
3. replaceState /done - replace history entry 2 - display confirmation

My problem is that I failed to pushState history entry 2 and entry 1 was getting replaced. It's a good idea to pushState on form submissions in all cases: it will add an entry to the history like a failed POST submission does in a traditional app.

Conclusion: should come with a warning that you should only render in in reaction to something that did a pushState, and that state will be replaced by the post-redirect state. It makes sense but it's easy to get wrong.

I'll add a documentation commit to the PR (perhaps not until this week-end).

[mjackson]

Ah! I didn't know you were using a <form>.

Agree it's easy to get this wrong. Maybe we could provide a <Form> addon for people that does this for them? Something like this:

class Form extends React.Component {
  static contextTypes = {
    router: React.PropTypes.object.isRequired
  }

  state = {
    error: null,
    result: null
  }

  handleSubmit = (event) => {
    event.preventDefault()

    const { router } = this.context

    // Manually PUSH to the next location like a regular <form> does.
    router.transitionTo(
      // When a <form> has no action attribute it submits to
      // the current URL.
      this.props.action || router.location.pathname
    )

    // Submit the form however you want.
    submitTheForm(event.target, (error, result) => {
      this.setState({ error, result })
    })
  }

  render() {
    // If we know the form is submitted we can do the <Redirect> here.
    if (this.state.error || this.state.result)
      return <Redirect to="/done" state={this.state}/>

    return (
      <form {...this.props} onSubmit={this.handleSubmit}/>
    )
  }
}

The main part we'd need to be careful about if we do give people a <Form> is to make the "submit" handling generic enough that it works for a wide variety of possible backends.

[timdorr]

I think getting into forms is going to be problematic. There are enough users of things like redux-form and other form libraries that you'll end up not being able to be used.

Perhaps a more generic navigation container? One that Link would use to do the actual navigation action. @ryanflorence had mentioned something like this to me a while ago, so he might have something laying around in progress for it.

[mjackson]

If we just provide components it should be trivial to integrate with our API no matter what you're using.

[aaugustin]

IMHO react-router should just recommend that users pushState when they handle to a user action, or rely on a component that does pushState for them, such as <Link>.

This is most likely to happen with <Redirect> because it's the only component in react-router that navigates with router.replaceWith.

Documenting why pushState must be called first and providing the <Redirect push> escape hatch sounds sufficent to me.

I'm afraid introducing a <Form> component would cause confusion.

[aaugustin]

You could give that <Form> as an example to illustrate how to pushState with router.transitionTo, though.

[aaugustin]

I rebased the PR and added a small paragraph to the docs for Redirect.

[yormi]

Oh ! That's what I needed !

I just wonder if it's possible to update the doc so that other users don't have to dig too deep !?

I know it's already written but is not live there: https://react-router.now.sh

Thanks for the good work guys :)