Make <Link> work with static containers

[gaearon]

Fixes #470, thereby unbreaking every project that uses React Redux with React Router.

This follows the approach described in #3340 (comment), but I’m happy for this to go into 3.0 rather than 2.x. I tested this on a few apps, and it works well in my testing. I’m happy to add as many additional unit tests as you like if this is something you are ready to consider to take in. I believe it’s going to take a huge pain point away from using these together, and I’m super excited about the possibility of this shipping.

Technical details as comments inline.

[taion]

The perf problem isn't setState() – the slow call is router.isActive.

[gaearon]

I understand, but we only call it once initially (which we would call anyway), and when the related props have changed (which again, we’d have to recompute anyway).

setState() however can be slow on big trees. I’ve seen 3x difference on large trees in other project solved by avoiding setState even despite further shouldComponentUpdate bailout. React places setState in a queue and marks subtrees as dirty so this might be relevant.

[gaearon]

(Previously, it would be called during render, but now it doesn’t, so it seems like the number of useful times is equivalent. As far as I can see any other wins from not calling it are the bug, not the feature 😄 )

[gaearon]

These conditions are moved from the render function.

[taion]

Doesn't this make the link always re-render twice on location updates then? Once with the old active state, once with the recalculated active state?

[gaearon]

In my testing this doesn’t seem to be the case. I think the listener fires before React has a chance to process setState in the router so it looks like everything happens in a single batch. I might be wrong about the exact mechanics though—just saying I don’t see duplicate renders in my example app.

[taion]

Note my caveat in #470 (comment) – if you hit this from clicking on a link, then stuff might well get batched together, but it won't happen if the transition is triggered from something other than a React event handler.

[gaearon]

Ahh. I get it now.

[taion]

I do not think this is the correct approach here.

It's difficult to allow multiple transition manager subscribers without edge cases (for example, right now, if you try to attach a second listener while the initial match is still running, that second listener will kick off a second match).

I feel like we can do this in a generic way by introducing a concept of a "subscribable" context, and applying this notion to context.router. Consumers of context.router (including the withRouter HoC as needed) can then subscribe to the router, keep track of e.g. the most recent processed update, and only re-render if the most recent update doesn't match the most recent rendered update.

This "subscribe and re-render only if last rendered update index doesn't match new update index" can be used generically across libraries that make use of this sort of context pattern, and lets us use much looser coupling of the behavior here.

[gaearon]

Consumers of context.router (including the withRouter HoC as needed)

IMO it would make sense to have this as the default behavior for withRouter then. What do you think?

[taion]

They should go together.

I'm actually a bit more concerned now about the case of how something like this works at all in the context of not using batched updates.

I feel like potentially having dozens of non-batched setState calls would be a bad idea.

[taion]

Though I do think it would be semver-breaking to make this the default (and to change router.listen, which IMO we shouldn't expose at all given its odd async semantics), and again I think this is something better configured via something like the context subscription HoC (or factored-out utility).

[gaearon]

I feel like potentially having dozens of non-batched setState calls would be a bad idea.

React Router can use ReactDOM.unstable_batchedUpdates like Relay does.
Also, correct is better than fast. Right now it’s widely broken. We can always provide an opt-out from this.

[gaearon]

I feel like we can do this in a generic way by introducing a concept of a "subscribable" context, and applying this notion to context.router.

Would this be any different from an event emitter?

[taion]

The only difference is on the subscriber side. You'd attach an update index to each update, then process the event only if the received update index doesn't match the most recently rendered update index.

That way the subscription ends up being a no-op and everything goes through the standard re-render when intervening SCU clauses are not causing issues.

[taion]

See #470 (comment) and #470 (comment).

This would wire up to <RouterContext>. There's nothing router-specific there, so it'd be reusable in other contexts and the code could live as a dependency of this library.

[gaearon]

Thanks for taking time! I’ll try to make it work. Who would know that router has changed? The router object is created in <Router> which is ignorant of the context, and the context lives in <RouterContext> which doesn’t know what constitutes the router change.

[taion]

<RouterContext> would be the right place to do this. It should only re-render when there's a router change. I guess for a paranoid check, compare this.props.routes to prevProps.routes or something? (I'm not sure I'd bother doing that.)

In general for this sort of container pattern, the context updates when the container re-renders, right? Or at least that's how it'd work without intervening SCUs? I'm thinking an HoC like:

function createContextProvider(name, contextType = React.PropTypes.any) {
  return class ContextProvider extends React.Component {
    static propTypes = {
      children: React.PropTypes.node.isRequired,
    };

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

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

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

      this.eventIndex = 0;
      this.listeners = [];
    }

    getChildContext() {
      return {
        [name]: {
          ...this.context[name],
          subscribe: this.subscribe,
          eventIndex: this.eventIndex,
        },
      };
    }

    componentWillReceiveProps() {
      ++this.eventIndex;
    }

    componentDidUpdate() {
      this.listeners.forEach(listener => listener(this.eventIndex));
    }

    subscribe = (listener) => {
      // No need to immediately call listener here.
      this.listeners.push(listener);

      return () => {
        this.listeners = this.listeners.filter(item => item !== listener);
      };
    };

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

Then in RouterContext.js, something like:

const RouterContextProvider = createContextProvider('router', routerShape);

// in render
return (
  <RouterContextProvider>
    {element}
  </RouterContextProvider>
);

Does this look generic enough? These two HoCs seem usable almost anywhere.

[taion]

I think the plan is to move to the router object being created/owned by <RouterContext>. That the <Router> owns that context object is more just so we can sanely do the deprecations in v2.x.