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
Add <Redirect push> and <Link replace> to override defaults #3903
Comments
We probably want to add a prop to A PR to add this would be great! |
I've been thinking <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/> |
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. |
Good, let's go with a boolean option and add it to I would like to explain in the documentation why 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 |
I also expected In this page from the docs it says:
If I can use context, shouldn't something like |
As far as I understand, using the context means: const {router} = this.context
router.transitionTo(...) |
Yes, I know I could do this. But it isn't a good practice to let users access
Providing some HOC such as In fact, it was already done in v2: #3350 |
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? |
#3912 implements what @ryanflorence and @timdorr described above. I wrote integration tests because I would prefer to make |
The default behavior of 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/react@15.3.2 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 :) |
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
client-side app User clicks
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 I'll add a documentation commit to the PR (perhaps not until this week-end). |
Ah! I didn't know you were using a Agree it's easy to get this wrong. Maybe we could provide a 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 |
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. |
If we just provide components it should be trivial to integrate with our API no matter what you're using. |
IMHO react-router should just recommend that users This is most likely to happen with Documenting why I'm afraid introducing a |
You could give that |
I rebased the PR and added a small paragraph to the docs for |
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 :) |
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>
):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:
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.
transitionTo
the default behavior of<Redirect>
, as it's less likely to corrupt history inadvertently thanreplaceWith
. By defaulting toreplaceWith
, 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.transitionTo
orreplaceWith
, perhaps by providing one component for each behavior, or a prop on Redirect to select the behavior. I'm unsure whenreplaceWith
would be appropriate, but I assume you had a reason to choose it and would like to preserve that possibility in the API.Thanks for your consideration! Let me know if some of these ideas seem worth discussing further.
The text was updated successfully, but these errors were encountered: