setState warning with ConnectedRouter

[mattfordham]

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 tocomponentWillMount.

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

[garbin]

same here, use ConnectedRouter, use dispatch(push('/')) in onClick anywhere, the warning will be reproduce

[sym900728]

I got the same warning. use the react-router-redux and Link

[boba-keyost]

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.

[timdorr]

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.

[marvinpinto]

This is why it's alpha 😄 We'll get this fixed soon enough.

What do you mean by this @timdorr?

Looking at the release tags, it seems like 7ce8ccf was pushed out as the proper (not-alpha, not-beta) version 4.0.0 as well as 5.0.0-alpha.1.

[Venryx]

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)

[Venryx]

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;
		}})
	);
};

[lost-osiris]

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

[inxilpro]

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.

[timdorr]

@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.

[Boorj]

@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.

[kn0ll]

@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?)

[inxilpro]

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?

[inxilpro]

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)
    }
  }
}

[inxilpro]

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.

[Patrik-Glendell]

@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 :)

[js3692]

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)

[timdorr]

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.