ActiveState changes don't propagate if any single parent in hierarchy declares shouldComponentUpdate

[gaearon]

If I have a component using ActiveState mixin and some of its parents declare shouldComponentUpdate, the child won't update when active state changes.

This is not normal because usually parent's shouldComponentUpdate doesn't magically cut the child off updates. If child listens to some store and calls setState, it will be updated. But this is not the case with context.

Even if parent's shouldComponentUpdate implementation takes nextContext into account, it won't even get nextContext if it doesn't declare contextTypes itself. So basically, for ActiveState to work in a project that heavily uses shallow-comparing shouldComponentUpdate or something similar, all ancestors of ActiveState-using component need to also have ActiveState.

See this React issue and this fiddle.

Obviously it's a React's problem per se but since router relies on context so much, it's worth documenting.

[gaearon]

Here's my temporary workaround (to be used instead of ActiveState):

// -----------------
// index.js
// -----------------
var Navigation = require('./Navigation');

function handleRouteChange() {
  Navigation.emitChange();
}

var routes = React.render(
  <Routes location='history' scrollBehavior='browser' onChange={handleRouteChange}>
    ...
  </Routes>,
  document.body
);

Navigation.injectInstance(routes);


// -----------------
// Navigation.js
// -----------------

'use strict';

var invariant = require('react/lib/invariant'),
    assign = require('react/lib/Object.assign'),
    PathStore = require('react-router/modules/stores/PathStore'),
    { EventEmitter } = require('events');

var CHANGE_EVENT = 'change',
    _routes;

/**
 * Provides react-router/Navigation API that can be used
 * from outside the components (e.g. from action creators).
 */
module.exports = assign({
  injectInstance(routes) {
    invariant(!_routes, 'Expected injection to happen once.');
    _routes = routes;
  },

  /**
   * Call this method from <Routes onChange /> handler to keep listeners in sync.
   */
  emitChange() {
    this.emit(CHANGE_EVENT);
  },

  addChangeListener(callback) {
    this.on(CHANGE_EVENT, callback);
  },

  removeChangeListener(callback) {
    this.removeListener(CHANGE_EVENT, callback);
  },

  getCurrentPath() {
    if (_routes) {
      return _routes.getCurrentPath();
    } else {
      // If _routes have not yet been injected, ask the Store
      return PathStore.getCurrentPath();
    }
  },

  makePath(to, params, query) {
    return _routes.makePath(to, params, query);
  },

  makeHref(to, params, query) {
    return _routes.makeHref(to, params, query);
  },

  transitionTo(to, params, query) {
    _routes.transitionTo(to, params, query);
  },

  replaceWith(to, params, query) {
    _routes.replaceWith(to, params, query);
  },

  goBack() {
    _routes.goBack();
  }
}, EventEmitter.prototype);



// -------------------
// ActiveStateMixin.js
// -------------------

'use strict';

var { PropTypes } = require('react'),
    Navigation = require('./Navigation');

/**
 * Provides functionality similar to react-router's ActiveState mixin
 * but without relying on context and woes associated with it:
 *
 * https://github.com/rackt/react-router/issues/470
 * https://github.com/facebook/react/issues/2517
 */
var ActiveStateMixin = {
  contextTypes: {

    // Only use context for a function that we know won't change.
    // Changes in context don't propagate well:
    // https://github.com/facebook/react/issues/2517

    makePath: PropTypes.func.isRequired
  },

  getRouterState() {
    return {
      currentPath: Navigation.getCurrentPath()
    };
  },

  getInitialState() {
    return this.getRouterState();
  },

  componentWillMount() {
    Navigation.addChangeListener(this.handleRouterStateChange);
  },

  componentWillUnmount() {
    Navigation.removeChangeListener(this.handleRouterStateChange);
  },

  handleRouterStateChange() {
    this.setState(this.getRouterState());
  },

  isActive: function (to, params, query) {
    return this.state.currentPath ===
           this.context.makePath(to, params, query);
  }
};

module.exports = ActiveStateMixin;

[gaearon]

As of RR 0.11, the easiest way to fix it is to have a Flux store for router state like in this tutorial. Then you can implement a custom State mixin that has exact same API as RR's State but is guaranteed to be up-to-date whenever router state changes:

'use strict';

var { State } = require('react-router'),
    RouterStore = require('../stores/RouterStore');

/**
 * Provides exact same functionality to react-router's ActiveState mixin
 * but without relying on context propagation that breaks shouldComponentUpdate:
 *
 * https://github.com/rackt/react-router/issues/470
 * https://github.com/facebook/react/issues/2517
 */
var RouterStateMixin = {
  mixins: [State],

  getRouterState() {
    return {
      routerState: RouterStore.getRouterState()
    };
  },

  getInitialState() {
    return this.getRouterState();
  },

  componentWillMount() {
    RouterStore.addChangeListener(this.handleRouterStateChange);
  },

  componentWillUnmount() {
    RouterStore.removeChangeListener(this.handleRouterStateChange);
  },

  handleRouterStateChange() {
    this.setState(this.getRouterState());
  }
};

module.exports = RouterStateMixin;

[mjackson]

@gaearon It should be noted that this solution will only work in a Flux environment, not on the server. That's the reason for using context instead of stores in the first place.

[gaearon]

It proxies to RR's proper State now, so it should “work” on server too in the sense that it wouldn't break, assuming components don't use this.state.routerState directly.

It doesn't rely on information from the store—the only reason routerState is there is to force re-render when necessary. Components would still call isActive etc.

[mjackson]

Is RouterStore something you made? I can't find where it's defined.

[mjackson]

Oh, nm. In the page you linked to @rpflorence is dispatching "router actions" ... which will be picked up by the RouterStore.

[gaearon]

Yeah, it's something along the lines of this tutorial.

index.js

router.run((Handler, state) => {
  RouterActionCreators.changeRoute(state);
  React.render(<Handler />, document.body);
});

RouterActionCreators.js

'use strict';

var AppDispatcher = require('../dispatcher/AppDispatcher'),
    ActionTypes = require('../constants/ActionTypes');

var RouterActionCreators = {
  changeRoute(routerState) {
    AppDispatcher.handleViewAction({
      type: ActionTypes.ROUTE_CHANGE,
      routerState: routerState
    });
  }
};

module.exports = RouterActionCreators;

RouterStore.js

'use strict';

var AppDispatcher = require('../dispatcher/AppDispatcher'),
    ActionTypes = require('../constants/ActionTypes'),
    { createStore } = require('../utils/StoreUtils');

var _routerState;

var RouterStore = createStore({
  getRouterState() {
    return _routerState;
  }
});

AppDispatcher.register((payload) => {
  var { action } = payload;

  switch (action.type) {
  case ActionTypes.ROUTE_CHANGE:
    _routerState = action.routerState;
    RouterStore.emitChange();
    break;
  }
});

module.exports = RouterStore;

[mjackson]

I still don't see how the RouterStore works in the server environment. Since it's a singleton, it's going to be shared across all your routers, which doesn't work if you're doing async transitions.

In order for us to be able to use the actions/dispatcher/store pattern server-side we'd have to have at least one store per router, which is essentially what context gives us.

[gaearon]

I still don't see how the RouterStore works in the server environment.

Maybe we're talking about different uses? I don't mean to use RouterStore as source of truth.

This is just a workaround I'm using until we get this shouldComponentUpdate behavior. The only reason for it to exist is to force re-rendering of components that aren't reached due to falsy shouldComponentUpdate up the tree. In the server scenario, components would use methods such as isActive which are still implemented by State via context.

My mixin proxies to State:

var RouterStateMixin = {
  mixins: [State],

but adds logic to make it shouldComponentUpdate-friendly.

This is kinda equivalent if it makes more sense:

var RouterStateMixin = {
  mixins: [State],

  componentDidMount() {
    RouterStore.addChangeListener(this._handleRouterStoreChange);
  },

  componentWillUnmount() {
    RouterStore.removeChangeListener(this._handleRouterStoreChange);
  },

  _handleRouterStoreChange() {
    this.forceUpdate();
  }
};

For the server side, it works exactly like State because server never reaches componentDidMount.

[mjackson]

Ah, yes. That makes sense now. Thank you for explaining.

I wonder if we should include this in Router.State for the sake of by-passing the problem you're talking about. As you say, it's something we'd only ever use client-side, and it seems to solve the problem nicely.

[gaearon]

I'd say wait for other reports of this issue. It doesn't seem like a lot of people are using shouldComponentUpdate in RR apps, and the issue is quite Google-able by “shouldcomponentupdate react router”. Perhaps solving it in the router is not worth the additional complexity.

[ryanflorence]

I still don't see how the RouterStore works in the server environment.

You'll have a Router.run in server.js and one in client.js, you won't even pull RourterStore into server.js

[mjackson]

@rpflorence That's not the way I'd do it.

I'd reuse the same Router.run on both the server and the client since the only thing that needs to change is the location (server-side uses a string path, client uses a Location object). Then I'd rely on the fact that server-side code will never actually call componentDidMount or componentWillUnmount, so the store won't ever actually be used. It's there in both. On the server it's just a no-op.

[ryanflorence]

Router.run lives inside of a server side handler, you also have to deal with dumping data into that template, etc. etc. etc. Its very different.