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.

[ryanflorence]

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

Says the guy who hasn't done it :P

[mjackson]

Haha. Even if I had two calls to Router.run in my app (which I don't, and yes, I've already done it in my app :P) I still wouldn't require('RouteStore') in either of them because this is stuff the router should be doing for me.

[ryanflorence]

what is even ActiveState anymore? ... since all this stuff is in props with v1.0, I think we're good.

[gaearon]

I don't think this is solved. (Sorry for re-opening if it is; I'm just afraid we'll forget this in 1.0.)

The Link still receives router via context and checks isActive directly. If any component with pure shouldComponentUpdate in the middle blocks the rendering, the Link state will be stale.

The real solution here is to expose something like subscribe(listener) on context.router that will call the callback any time the router receives new props. This will allow Link to subscribe to that state independently and run setState({ isActive: context.router.isActive(path) }) any time the router emits a change.

It doesn't sound “beautiful” but it solves the problem. This is how I do it in Redux too.

@mjackson ?

[gaearon]

In other words, don't use the context to pass data, even @sebmarkbage said this. :-)

Use the context to wire DI. For passing data not through props, you still need some sideways loading mechanism, and today the best we have is manual subscription model + setState.

[sebmarkbage]

Actually, I think the opposite. You SHOULD pass data through context, not subscriptions. Let React optimize the update model so that it works despite shouldComponentUpdate. React can chose to use subscriptions to do that, facebook/react#3973 or we can use a strategy that walks the tree to find places that needs to be updated. We can make tradeoffs depending on other work that is going on etc.

[gaearon]

Thanks for chiming in! It will be great when React solves that.

But right now people do extreme things (that's a base Component class for the whole framework!) just to get around the issue, and many aren't even aware that the issue exists.

It's easy to fix on the router level. Let _subscribe be a private API for now if we don't want people to depend on it. When React solves this, we can bump the minimum version and remove the hack.

I really want RR 1.0 to fix this, and if it comes out before React fixes sCU with context, I'd vote for doing a sideways subscription.

[mjackson]

@gaearon Sure, let's do as you suggest for now. I have a feeling that React 0.14 is going to be released about the same time as we're ready for a 1.0 though, so we might be doing unnecessary work sidestepping an issue that will soon be fixed for us.

Maybe @zpao can give us an idea about when 0.14 stable is going to land..

[gaearon]

Context is still unofficial API so I’d avoid creating libraries that rely on it as part of their public API. If context changes, React Router or React Redux will shield its consumers, but this would be harder for a library that has a primary purpose of doing something with context. This might also send a wrong signal (“this is how you deal with context”) when all we want to do is to work around a bug in an unofficial feature of React. I’d rather prefer this stays in the router, or at least starts there. We can always extract later.

[taion]

I don't know, it just seems odd to me to have code per se here that largely accomplishes the goal of Redux/Relay compatibility. I don't think the experimental nature of context is relevant for a specialized library like the one I'm proposing – it'd only be relevant for people who were already using context, who would be broken anyway by upstream changes in context.

[gaearon]

largely accomplishes the goal of Redux/Relay compatibility.

I might understand it incorrectly but it feels to me that it’s React Router that chooses to rely on a broken React feature (updates to context), not Redux or Relay. It’s a conscious choice that can be avoided (e.g. React Redux avoids it exactly the same way by putting subscribe rather than the value into the context), so IMO it’s fitting that the library that makes a conscious choice like this also implements a workaround for the problems this choice causes in a larger ecosystem.

I’m happy to take a stab at a PR for this. Not blaming anyone. I just don’t see what’s wrong with having this code here.

[taion]

In practical terms this is something that comes up everywhere – not just React Router but also React Intl, and also any form library that wants to give the user enough flexibility around form layout such that form elements don't have to be immediate children (but don't depend on a separate data store).

I think factoring this support out into a separate library gives us cleaner and more reusable way to implement this feature, and gives us a clear domain separation between "this is the workaround for context + SCU" and "this is what React Router needs to do to be Redux-friendly".

What do you think is the downside of pulling this support out into a separate library, then using that library as a dependency for React Router? I think we end up at roughly the same place, but we make life easier for other users of React context.

[taion]

function createContextSubscriber(name, contextType = React.PropTypes.any) {
  return class ContextSubscriber extends React.Component {
    static propTypes = {
      children: React.PropTypes.isRequired,
    }

    static contextTypes = {
      [name]: contextType,
    };

    constructor(props, context) {
      super(props, context);

      this.unsubscribe = null;
      this.lastRenderedEventIndex = null;
    }

    componentDidMount() {
      this.unsubscribe = this.context[name].subscribe(eventIndex => {
        if (eventIndex !== this.lastRenderedEventIndex) {
          this.forceUpdate();
        }
      });
    }

    componentWillUnmount() {
      if (!this.unsubscribe) {
        return;
      }

      this.unsubscribe();
      this.unsubscribe = null;
    }

    render() {
      this.lastRenderedEventIndex = this.context[name].eventIndex;

      return this.props.children;
    }
  };
}

[taion]

Actually, better implementation:

function createContextSubscriber(name, contextType = React.PropTypes.any) {
  return class ContextSubscriber extends React.Component {
    static propTypes = {
      children: React.PropTypes.isRequired,
    }

    static contextTypes = {
      [name]: contextType,
    };

    constructor(props, context) {
      super(props, context);

      this.state = {
        lastRenderedEventIndex: context.eventIndex,
      };

      this.unsubscribe = null;
    }

    componentDidMount() {
      this.unsubscribe = this.context[name].subscribe(eventIndex => {
        if (eventIndex !== this.state.lastRenderedEventIndex) {
          this.setState({ lastRenderedEventIndex: eventIndex });
        }
      });
    }

    componentWillReceiveProps() {
      this.setState({ lastRenderedEventIndex: this.context.eventIndex });
    }

    componentWillUnmount() {
      if (!this.unsubscribe) {
        return;
      }

      this.unsubscribe();
      this.unsubscribe = null;
    }

    render() {
      return this.props.children;
    }
  };
}

[gaearon]

#3430

[buzinas]

For anyone who's already using React Router Redux, I made a pretty simple Link component and it's working on two apps I'm working on:

import React from 'react';
import { connect } from 'react-redux';
import { Link } from 'react-router';

const ReduxLink = ({ className = '', activeClassName, to, path, children }) => {
  let finalClassName = className.split(' ');

  if (activeClassName && path === to) {
    finalClassName.push(activeClassName);
  }

  return (
    <Link to={to} className={finalClassName.join(' ')}>
      {children}
    </Link>
  );
};

export default connect(
  (state, ownProps) => Object.assign({}, ownProps, { path: state.routing.locationBeforeTransitions.pathname })
)(ReduxLink);

Btw, it can replace both Link and IndexLink components.

[gaearon]

@buzinas

Note however that, as { path: state.routing.locationBeforeTransitions.pathname } implies, this won’t display the correct route during async transitions.

[buzinas]

@gaearon Yeah, I know. And it also changes the activeClassName before animations, but at least it's a fallback until this is solved "natively" in react-router. Same to your gist, but probably in a more performant way.

[timdorr]

Fixed in #3430 (and followup commits by @taion)

[gaearon]

This is now out in 3.0.0-alpha.1. Please give it a try!

[norkfy]

Yahhooo!!

[taion]

@gaearon

This leads to a pretty interesting edge case when working with Relay.

<Relay.Renderer> renders its loading state asynchronously. As such, the "immediate" update of Relay containers to indicate loading state after a Relay query config update isn't synchronous.

What happens with this logic, then, when using Relay (either with react-router-relay or otherwise) is:

1. Links will all separately rerender their new active states (if relevant) synchronously
   1. This is disconnected from any of their parents, since nothing else is rerendering synchronously
2. Then, Relay components rerender again immediately afterward

This is not ideal for performance – it results in rendering the links an extra time for (at best) no benefit.

I don't think there's a good way around this with the current implementation, since the RR v2/v3 architecture treats all this data loading stuff as entirely downstream, and doesn't give the router any way to know about it. It's really just a caveat emptor now.

For the time being, I'm going to recommend that users of react-router-relay continue to use React Router v2 to avoid this performance quirk.