Closed mattfordham closed 7 years ago
same here, use ConnectedRouter, use dispatch(push('/')) in onClick anywhere, the warning will be reproduce
I got the same warning. use the react-router-redux and Link
This issue caused by ConnectedRouter render method. It seems, that there should be component(with componentWillRecieveProps or componentDidUpdate methods) instead of render function for inner Route component.
Well, considering that's the primary function of <ConnectedRouter>
, just removing that line isn't going to work... 😉
But yeah, I can see how I'm violating the pureness of the render method. Looks like I need to extract an internal component so I can hook up the lifecycle methods and dispatch freely from within there. I'll want to combine the efforts of #4717 so we can also get time travel support mixed in.
This is why it's alpha 😄 We'll get this fixed soon enough.
This is why it's alpha :smile: We'll get this fixed soon enough.
What do you mean by this @timdorr?
Looking at the release tags, it seems like https://github.com/ReactTraining/react-router/commit/7ce8ccfe0f38a1381f5456ee86c8a3358a1f1c78 was pushed out as the proper (not-alpha, not-beta) version 4.0.0 as well as 5.0.0-alpha.1.
Same issue here. Glad I'm not alone. (it's terrible when you have a hard-to-fix bug, search it up, and find... nothing)
This may have bad side-effects, but a temporary solution:
Open up the ConnectedRouter.js
file, and wrap the store.dispatch
call in a setTimeout([...], 0)
call, like so:
ConnectedRouter.prototype.render = function render() {
[...]
return _react2.default.createElement(
[...]
_react2.default.createElement(_reactRouter.Route, { render: function render(_ref) {
var location = _ref.location;
setTimeout(()=> {
store.dispatch({
type: _reducer.LOCATION_CHANGE,
payload: location,
});
});
return children;
}})
);
};
For those interested I have a temporary fix. Basically I created a component to check if the location on history had changed and if it had then it would call store.dispatch
using the proper component life cycles. For now though I copied the ConnectedRouter into my own component so I can import/modify it else where until this is fixed.
Not sure if this was the direction you wanted to go for a PR but at least it's a work around.
import invariant from 'invariant'
import React, { Component, PropTypes } from 'react'
import { Router, Route } from 'react-router-dom'
class ConnectedRouter extends Component {
static propTypes = {
store: PropTypes.object,
history: PropTypes.object,
children: PropTypes.node
}
static contextTypes = {
store: PropTypes.object
}
componentWillMount() {
const { children } = this.props
invariant(
children == null || React.Children.count(children) === 1,
'A <ConnectedRouter> may have only one child element'
)
}
render() {
const { store:propsStore, history, children, ...props } = this.props
let store = propsStore || this.context.store
return (
<Router {...props} history={history}>
<Route render={({ location }) => {
return <MountedRoute store={store} location={location} children={children} />
}}/>
</Router>
)
}
}
class MountedRoute extends Component {
componentWillMount() {
this.location = this.props.location;
this.store = this.props.store;
this.props.store.dispatch({
type: '@@router/LOCATION_CHANGE',
payload: this.props.location
})
}
componentWillUpdate(nextProps) {
this.store = nextProps.store;
if (this.location.pathname != nextProps.location.pathname) {
this.location = nextProps.location;
this.store.dispatch({
type: '@@router/LOCATION_CHANGE',
payload: nextProps.location
})
}
}
render() {
return this.props.children;
}
}
export default ConnectedRouter
Fundamentally, this component just needs to dispatch a LOCATION_CHANGE
action when the location changes. My PR does that by subscribing directly to the history
.
@marvinpinto That was 4.0.0 for only the main packages (react-router, react-router-dom, and react-router-native). react-router-redux already has a 4.0.0 release from before, so this rewrite will be ahead of the other packages and version itself at its own pace.
@timdorr Can you please also provide for immutable usage case? Here's the desciption ->react-router-redux + react-immutable, take a look at the last listing.
Previously selectLocationState
was modified to use get('routing')
, hope in ConnectedRouter there will be ability to do the same.
@lost-osiris your snippet is a helpful attempt and seemed to work at first.
but, it seems to break dispatching push actions. using the supplied ConnectedRouter, dispatch(push()) will change the URL and the urls state will be reflected in Redux.
using your ConnectedRouter, dispatch(push()) will only change the URL, but breaks the behavior that updates the state based on location change (i assume your ConnectedRouter breaks the router middleware?)
Does it, perhaps, make sense to break this functionality up? So that there's are separate reducers and action dispatchers, and perhaps a way to hook into the action dispatcher to support different use cases like devtools or immutable?
Oops, I was writing that on my phone and now I realize that that already is basically how it works :) What's the reason to use ConnectedRouter
at all? Why not just add a listener here:
import { CALL_HISTORY_METHOD, LOCATION_CHANGE } from './actions'
export default function routerMiddleware(history) {
return ({ dispatch }) => {
history.listen(payload => dispatch({
type: LOCATION_CHANGE,
payload
}))
return next => action => {
if (action.type !== CALL_HISTORY_METHOD) {
return next(action)
}
const { payload: { method, args } } = action
history[method](...args)
}
}
}
I just submitted another PR with this implementation. In my opinion, it's much cleaner. Effectively implements all of ConnectedRouter
in 4 lines of middleware code.
@inxilpro, trying your PR im not getting the location from the browser into redux at all, do you still use the Connect statement for your children ?
its the initial state of the location when first loading the application i want fetched from the browser as it is in connectedRouter, once clicked away your solution seems to work fine :)
Building on @lost-osiris's idea, here's another attempt that fixes the comments above:
// In your root application container:
import { Provider } from 'react-redux'
import { Router, Route } from 'react-router'
// import { ConnectedRouter } from 'react-router-redux'
import LocationSync from './LocationSync'
[...]
render () {
const { store, history } = this.props
const Routes = createRoutes(store)
// After update: Replace <Router> [...] </Router> with
// <ConnectedRouter history={history} children={Routes} />
// as suggested in the README.
return (
<Provider store={store}>
<Router history={history}>
<Route render={({ location }) => <LocationSync location={location} children={Routes} />} />
</Router>
</Provider>
)
}
// Then in LocationSync.js:
import { Component, PropTypes } from 'react'
import { connect } from 'react-redux'
const LOCATION_CHANGE = '@@router/LOCATION_CHANGE'
/**
* Temporary container element until "ConnectedRouter" is fixed.
* See https://github.com/ReactTraining/react-router/issues/4713
* and PR https://github.com/ReactTraining/react-router/pull/4717.
*
* Follow TODO item in RootContainer.js and delete this file after.
*/
class LocationSync extends Component {
constructor (props) {
super(props)
this.dispatchLocation = this.dispatchLocation.bind(this)
this.dispatchLocation(null, props.location)
}
componentWillReceiveProps (nextProps) {
this.dispatchLocation(this.props.location, nextProps.location)
}
dispatchLocation (prevLocation, nextLocation) {
const shouldDispatchLocation = (prev, next) => prev.pathname !== next.pathname
if (!prevLocation || shouldDispatchLocation(prevLocation, nextLocation)) {
console.log('[LOCATION DISPATCH]: ', location)
this.props.dispatch({
type: LOCATION_CHANGE,
payload: location
})
}
}
render () {
return this.props.children
}
}
LocationSync.propTypes = {
dispatch: PropTypes.func.isRequired,
location: PropTypes.object.isRequired,
children: PropTypes.node.isRequired
}
export default connect()(LocationSync)
Just pushed alpha 4 that will fix this issue. Still have to get time travel support in there, but this should be a more solid foundation for things going forward. I'll start pulling in some things from 4.0.8, since a lot of it is going to be the same.
Version
react-router: 4.0.0 react-router-redux: 5.0.0alpha2
Steps to reproduce
Use ConnectedRouter ;)
Expected Behavior
No warning.
Actual Behavior
Using ConnectedRouter leads to the following warning on every route change:
Warning: setState(...): Cannot update during an existing state transition (such as within
renderor another component's constructor). Render methods should be a pure function of props and state; constructor side-effects are an anti-pattern, but can be moved to
componentWillMount.
From what I can tell from the logs, it may be due to the
store.dispatch
here: https://github.com/ReactTraining/react-router/blob/master/packages/react-router-redux/modules/ConnectedRouter.js#L24