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

Best practice to redirect within action creator? #3498

Closed
olalonde opened this Issue May 27, 2016 · 17 comments

Comments

Projects
None yet
@olalonde

olalonde commented May 27, 2016

There seems to be two ways to redirect from an action creator (react-redux):

  1. Use withRouter() on component which calls the action creator and pass props.router to the action creator (e.g. dispatch(login(username, password, this.props.router)) and router.push('/success') inside my action creator)

  2. import { browserHistory } from 'react-router' inside the action creator file and browserHistory.push('/success') as mentioned here: https://github.com/reactjs/react-router/blob/master/docs/guides/NavigatingOutsideOfComponents.md

I was wondering which one would be preferable.

@timdorr

This comment has been minimized.

Show comment
Hide comment
@timdorr

timdorr May 27, 2016

Collaborator

You're calling the exact same function in both cases. But since you might use your action outside of the React context, you should probably use browserHistory.

Collaborator

timdorr commented May 27, 2016

You're calling the exact same function in both cases. But since you might use your action outside of the React context, you should probably use browserHistory.

@timdorr timdorr closed this May 27, 2016

@murashki

This comment has been minimized.

Show comment
Hide comment
@murashki

murashki Nov 9, 2016

Guys, what about 4.0.0 version?

murashki commented Nov 9, 2016

Guys, what about 4.0.0 version?

@malimichael

This comment has been minimized.

Show comment
Hide comment
@malimichael

malimichael Jan 16, 2017

How do we do this in v4?

How do we do this in v4?

@pshrmn

This comment has been minimized.

Show comment
Hide comment
@pshrmn

pshrmn Jan 16, 2017

Collaborator

Starting with the beta, the answer will be the same. If you need to navigate outside of React, you can create your own history instance and import it throughout your project.

Collaborator

pshrmn commented Jan 16, 2017

Starting with the beta, the answer will be the same. If you need to navigate outside of React, you can create your own history instance and import it throughout your project.

@wjin0352

This comment has been minimized.

Show comment
Hide comment
@wjin0352

wjin0352 Apr 19, 2017

How do we do this in v4???

How do we do this in v4???

@thekarel

This comment has been minimized.

Show comment
Hide comment
@thekarel

thekarel Apr 19, 2017

@wjin0352

The simplest working example:

history.js:

import createHistory from 'history/createHashHistory'

export default createHistory()

actions.js:

import history from './history'

history.push('/abc')

@wjin0352

The simplest working example:

history.js:

import createHistory from 'history/createHashHistory'

export default createHistory()

actions.js:

import history from './history'

history.push('/abc')
@Aprillion

This comment has been minimized.

Show comment
Hide comment
@Aprillion

Aprillion May 9, 2017

Contributor

@thekarel unfortunately, react-router-dom does not re-render the component that I pass to a Route when using push this way, but it works (i.e. re-renders the component) when I pass this.context.router.history.push into my action creator...

is that a general problem or only happening in my use case => should I create a simplified test case?

Contributor

Aprillion commented May 9, 2017

@thekarel unfortunately, react-router-dom does not re-render the component that I pass to a Route when using push this way, but it works (i.e. re-renders the component) when I pass this.context.router.history.push into my action creator...

is that a general problem or only happening in my use case => should I create a simplified test case?

@m-mik

This comment has been minimized.

Show comment
Hide comment
@m-mik

m-mik May 11, 2017

@Aprillion That's the expected behavior. Take a look at: #4059 (comment)

  1. Creating a new browserHistory won't work because BrowserRouter creates its own history instance, and listens for changes on that. So a different instance will change the url but not update the BrowserRouter.
  1. browserHistory is not exposed by react-router in v4, only in v2.

If you want the full history object you can also grab that off context like router.
this.context.history.push('/path')

m-mik commented May 11, 2017

@Aprillion That's the expected behavior. Take a look at: #4059 (comment)

  1. Creating a new browserHistory won't work because BrowserRouter creates its own history instance, and listens for changes on that. So a different instance will change the url but not update the BrowserRouter.
  1. browserHistory is not exposed by react-router in v4, only in v2.

If you want the full history object you can also grab that off context like router.
this.context.history.push('/path')

@Aprillion

This comment has been minimized.

Show comment
Hide comment
@Aprillion

Aprillion May 12, 2017

Contributor

@m-mik I understand that 2 instances of browserHistory would not communicate, but it was definitely NOT my expectation that creating single instance directly from history module, importing to multiple files, using it as <Router history={myHistory} and then running myHistory.push in another file somehow does not re-render the routed component in v4

Contributor

Aprillion commented May 12, 2017

@m-mik I understand that 2 instances of browserHistory would not communicate, but it was definitely NOT my expectation that creating single instance directly from history module, importing to multiple files, using it as <Router history={myHistory} and then running myHistory.push in another file somehow does not re-render the routed component in v4

@m-mik

This comment has been minimized.

Show comment
Hide comment
@m-mik

m-mik May 12, 2017

@Aprillion Are you sure you're using Router, and not BrowserRouter when passing the history instance? Perhaps, you're importing BrowserRouter as Router?

import { BrowserRouter as Router } from 'react-router-dom'
...
<Router history={history}> // this won't work, BrowserHistory does not accept history property

history.js

import createHistory from 'history/createBrowserHistory'

export default createHistory()

Root.jsx

import { Router, Route } from 'react-router-dom'
import history from './history'
...
<Router history={history}>
 <Route path="/test" component={Test}/>
</Router>

another_file.js

import history from './history' 

history.push('/test') // this should change the url and re-render Test component

m-mik commented May 12, 2017

@Aprillion Are you sure you're using Router, and not BrowserRouter when passing the history instance? Perhaps, you're importing BrowserRouter as Router?

import { BrowserRouter as Router } from 'react-router-dom'
...
<Router history={history}> // this won't work, BrowserHistory does not accept history property

history.js

import createHistory from 'history/createBrowserHistory'

export default createHistory()

Root.jsx

import { Router, Route } from 'react-router-dom'
import history from './history'
...
<Router history={history}>
 <Route path="/test" component={Test}/>
</Router>

another_file.js

import history from './history' 

history.push('/test') // this should change the url and re-render Test component
@Aprillion

This comment has been minimized.

Show comment
Hide comment
@Aprillion

Aprillion May 12, 2017

Contributor

that was probably it.. I didn't get any proptypes error, so I forgot to check.. thanks @m-mik for the tip, I will try again!

Contributor

Aprillion commented May 12, 2017

that was probably it.. I didn't get any proptypes error, so I forgot to check.. thanks @m-mik for the tip, I will try again!

@vvinhas

This comment has been minimized.

Show comment
Hide comment
@vvinhas

vvinhas Oct 18, 2017

Hey guys, is history navigation inside actions considered a good practice? I try to avoid it at all costs but I'm in a situation where this approach would be easier to implement.

vvinhas commented Oct 18, 2017

Hey guys, is history navigation inside actions considered a good practice? I try to avoid it at all costs but I'm in a situation where this approach would be easier to implement.

@Aprillion

This comment has been minimized.

Show comment
Hide comment
@Aprillion

Aprillion Oct 19, 2017

Contributor

@vvinhas I probably don't understand the question, for me history navigation can ONLY happen as an action.

e.g. <Link>is just a component that wraps the onClick event handler creating a history action (push). so a custom history action would be similar but using a custom component instead of a library-provided one.

I can't imagine when it would make sense to navigate back inside render, constructor, redux reducer, selector or something else, only inside an event handler => action creator.

But if you have a different concrete example..?

Contributor

Aprillion commented Oct 19, 2017

@vvinhas I probably don't understand the question, for me history navigation can ONLY happen as an action.

e.g. <Link>is just a component that wraps the onClick event handler creating a history action (push). so a custom history action would be similar but using a custom component instead of a library-provided one.

I can't imagine when it would make sense to navigate back inside render, constructor, redux reducer, selector or something else, only inside an event handler => action creator.

But if you have a different concrete example..?

@vvinhas

This comment has been minimized.

Show comment
Hide comment
@vvinhas

vvinhas Oct 20, 2017

Hey @Aprillion! Thanks for your reply!

My case is an auto redirect, based on a response from a Http Request. I know it's easier to put history.push into my thunk, but generally, I let my components decide when to redirect the user based on Redux State, wrapping withRouter around it and using the history prop.

I'm actually asking it honestly, I don't know if it's a common practice to put history.push inside a thunk.

vvinhas commented Oct 20, 2017

Hey @Aprillion! Thanks for your reply!

My case is an auto redirect, based on a response from a Http Request. I know it's easier to put history.push into my thunk, but generally, I let my components decide when to redirect the user based on Redux State, wrapping withRouter around it and using the history prop.

I'm actually asking it honestly, I don't know if it's a common practice to put history.push inside a thunk.

@Aprillion

This comment has been minimized.

Show comment
Hide comment
@Aprillion

Aprillion Oct 20, 2017

Contributor

Imagine new team members arriving and copy&pasting some pattern to other places in the code. Then 6 months later you get a bug report that some redirect is incorrect. Will you be able to debug the issue? If yes, then it's a good pattern for you :)

Not speaking for the whole community though, just how I would think about it.

Contributor

Aprillion commented Oct 20, 2017

Imagine new team members arriving and copy&pasting some pattern to other places in the code. Then 6 months later you get a bug report that some redirect is incorrect. Will you be able to debug the issue? If yes, then it's a good pattern for you :)

Not speaking for the whole community though, just how I would think about it.

@thekarel

This comment has been minimized.

Show comment
Hide comment
@thekarel

thekarel Oct 21, 2017

I let my components decide when to redirect the user based on Redux State

I would not recommend that practice. Components are either "dumb" and simply render a view based on data or "higher level" i.e. they connect the store's data to the dumb components. Other than that, your business logic should live in the store and side-effects handled in actions.

I let my components decide when to redirect the user based on Redux State

I would not recommend that practice. Components are either "dumb" and simply render a view based on data or "higher level" i.e. they connect the store's data to the dumb components. Other than that, your business logic should live in the store and side-effects handled in actions.

@EzeRangel

This comment has been minimized.

Show comment
Hide comment
@EzeRangel

EzeRangel Apr 24, 2018

Why not just make an static method? Router.push('/admin/')

Why not just make an static method? Router.push('/admin/')

@lock lock bot locked as resolved and limited conversation to collaborators Jun 23, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.