React-router: 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, https://github.com/facebook/react/pull/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]

I'm not sure it's part of 0.14. It's definitely an ongoing effort but I don't remember folk committing to ship it soon.

[gaearon]

I removed react-0.14 label because this will still be broken in 0.14.
It's a bug and we need to fix it.

(React's bug, but it's still up to us to work around it)

[mjackson]

I believe this is fixed by #1821

[gaearon]

Reopening because #1821 was reverted in 50372fe249cae70d768fcf19ead93423ed8175f0.

[mjackson]

Yep, thanks @gaearon

[taion]

I'm not 100% convinced that this should be fixed in React Router. For example, PureRenderMixin explicitly says

Furthermore, shouldComponentUpdate skips updates for the whole component subtree. Make sure all the children components are also "pure".

I actually think it's a little weird that Flux implementations with listeners bypass shouldComponentUpdate entirely, even if I use something like a StaticContainer where SCU just returns false and I don't want the subtree to re-render at all.

That said, it looks like it'd be pretty easy to salvage #1821 and address the concerns in https://github.com/rackt/react-router/pull/1821#issuecomment-139656332. Just keep track of the last rendered "active" state in either render or componentDidMount / componentDidUpdate as this._lastRenderedActive or something, then this.forceUpdate() in this.handleHistoryChange if the desired active state doesn't match the last rendered active state.

I don't think we _should_, because I want shouldComponentUpdate returning false to actually mean "don't update this subtree". For example, react-router-relay keeps Relay-ish default behavior of not updating any child route components at all until Relay's data fetching completes, and I'd be a little annoyed if the active state on all of my Links updated while nothing else did.

[gaearon]

PureRenderMixin saying so is just an artifact of React docs being silent about the context.
The context is documented now as of a few days ago, and it mentions the underlying issue:

If a context value provided by a component changes, descendants that use that value won't update if an intermediate parent returns false from shouldComponentUpdate. See issue facebook/react#2517 for more details.

This is React's bug, but I think Router should fix it because it _can_ fix it.

I actually think it's a little weird that Flux implementations with listeners bypass shouldComponentUpdate entirely, even if I use something like a StaticContainer where SCU just returns false and I don't want the subtree to re-render at all.

It's only weird if you don't take context in the picture.

I don't think we should, because I want shouldComponentUpdate returning false to actually mean "don't update this subtree". For example, react-router-relay keeps Relay-ish default behavior of not updating any child route components at all until Relay's data fetching completes, and I'd be a little annoyed if the active state on all of my Links updated while nothing else did.

Then no third party component can ever safely implement shouldComponentUpdate() _for its own logic_ because it might break the rendering of something context-reliant below. This totally breaks the utility of shouldComponentUpdate() for third-party component developers because they never know if you plan to use context or not.

This is also inconsistent with how local state works. If you setState() somewhere under a false shouldComponentUpdate(), it will still update. How is context different in this regard?

The usage pattern you're describing seems wrong to me. shouldComponentUpdate() is meant to be a _rendering optimization_—not a way to “block things below”. The things below can be stateful or include sideway data. Wouldn't it be weird if changing a localization language would only propagate to the parts of the UI that weren't in loading state? If you want to "freeze" the UI, the right way to do that is to have a contract where you pass deliberately "frozen" (past) props down—not a shouldComponentUpdate() block.

[gaearon]

That said, it looks like it'd be pretty easy to salvage #1821 and address the concerns in #1821 (comment). Just keep track of the last rendered "active" state in either render or componentDidMount / componentDidUpdate as this._lastRenderedActive or something, then this.forceUpdate() in this.handleHistoryChange if the desired active state doesn't match the last rendered active state.

If you can salvage this without breaking stuff, please do!

[taion]

@gaearon

Nevertheless StaticContainer exists and it's used in Relay for exactly this purpose.

I think it's a valid use of shouldComponentUpdate to control in general whether and when I want my component to update (per the name), rather than just for doing pure render optimizations. That's really more of a question for the thread on facebook/react, though.

That said, I am of the opinion that we should not work around this in React Router by default. Perhaps this could be controlled by an additional prop on the link. Adding this behavior will in fact break me.

[taion]

I vote for pushing this back from 1.0.0-rc4. This is a fairly thorny issue, and I don't think there's a good general solution that will work in all cases.

[knowbody]

done @taion

[ryanflorence]

The symptom here is that some links won't indicate they are active if they are behind a shouldComponentUpdate.

Because most links that deep in the app don't care if they are active, I'm closing this until we get more noise around it being an issue in real apps somebody is trying to ship.

Also, seems like a <SubscriberLink/> could pull history off context and subscribe to changes if it really needed to survive a shouldComponentUpdate, yeah?

[ryanflorence]

I'm closing this until we get more noise around it being an issue in real apps somebody is trying to ship.

Just learned that redux implements pure render on every connected thing, which means we're going to get a lot of noise.

By defaulting that way, any library that needs context (like theming, localization, routing, to name a few valid use cases) will need to dance around this _because of redux_, not because the apps actually have performance bottlenecks.

I think redux should not implement pure: true by default, VDOM is almost always fast enough, let people opt-in to problem areas of the app and cause the least amount of surprise for app developers, and not require every library developer to make adjustments because of redux's arguably incorrect default.

thoughts @gaearon?

[gaearon]

I think redux should not implement pure: true by default

I don't agree. React never encouraged libraries to depend on context, and _those that do can always get around the issue by passing subscriptions in context instead of data_. That's why I ranted about Link behavior before. This needs to be addressed by library authors because the user might optimize the app at some point, and Link and similar components will _break anyway_. If Redux defaults to opt-out, this will provide people with less incentives to finally fix it—either in libraries, or in React itself.

Let me note that Flux Utils also has pure: true by default. And so does Relay. It's a sensible default. Libraries relying on bug-free context change propagation are the ones at fault here.

VDOM is almost always fast enough

Not true when we're talking single atom state because it's just so much pressure on VDOM. People _are_ having perf issues (e.g. with forms), making perf opt in will make matters worse.

[taion]

Let's consolidate discussion here. This is a rehash of https://github.com/rackt/react-router/issues/2483#issuecomment-154230396, but the other problem is that I don't see a way to animate route components with <Link>s transitioning out if this logic is in place.

I really don't think there's a good solution available until e.g. shouldComponentUpdate gets a way to explicitly handle context updates. If necessary React Router (or even an extension package) can offer a subscribing <Link> component, but as-is, some set of use cases will break if we make the change to <Link> here.

Granted, a different set of use cases will un-break, but it's a tradeoff; it's not obvious that one solution or another is better.

[taion]

@gaearon

RE: https://github.com/rackt/react-router/issues/2483#issuecomment-154232682

It's new as of a few days ago; the animation example previously worked correctly by coincidence due to a bug in ReactReconciler with context, and this was required to make it work as intended again.

Can you explain what you mean by "a nested <RoutingContext>"? The component hierarchy looks something like

Router > RoutingContext > App > ReactCSSTransitionGroup > [Page1, Page2]

Even if we replace the StaticContainer with a separate routing context interceptor, if the <Link> components belonging to the component that is transitioning out is directly subscribed to the history object, they will still update their active state, which we don't want.

[gaearon]

Even if we replace the StaticContainer with a separate routing context interceptor, if the components belonging to the component that is transitioning out is directly subscribed to the history object, they will still update their active state, which we don't want.

Why would they be subscribed to the history object though? I was thinking of having another prop on the context, like subscribeToHistory. By default the <RoutingContext> sets it up to use history, but we can override it in the interceptor to return a canned value (e.g. a no-op), just like we can provide a fake location below.

[taion]

Wouldn't the <Link>s already be set up to subscribe to the real history object, though? i.e. the one that had already changed? It sounds like for this to work, the <Link>s would have to swap out their real subscription for a fake one.

[gaearon]

If they are already inside a transition group (and our custom wrapper), wouldn't they be created already being inside our custom wrapper that hijacks the context? React doesn't support reparenting, so they can't jump in the hierarchy.

[taion]

I think I understand what you mean - essentially create a transition-aware subscriber that detects when it's transitioning out and stops emitting events. I think it's going to be quite tricky and finicky in practice, though.

I think the main point is that I don't see the benefit here as being commensurate with the technical challenge involved in essentially building our own state management system that sits parallel to React's props and context.

[gaearon]

I think the main point is that I don't see the benefit here as being commensurate with the technical challenge involved in essentially building our own state management system that sits parallel to React's props and context.

I wouldn't call it a state system. You can use cloneElement to clone the props. Wrapping things in yet another <RoutingContext> with constant unchanging values seems like exactly the semantics you want to achieve in terms of context without introducing additional new primitives.

[taion]

The issue isn't the <RoutingContext> - it's the use of something like subscribeToHistory on <Link> to pull in the history state, rather than context itself.

I feel like I'd be more inclined to invert this problem and have e.g. a <LiveRoutingContext> that can sit under components with shouldComponentUpdate and itself "re-broadcast" updates to history.

[taion]

We should consider making a react-router-addons-live-link that implements this behavior.

[ryanflorence]

I'm sold. I think we should just have it subscribe to location changes as the default behavior.

[taion]

There's a significant part of me that wants to do what RN's experimental navigation does and use a reducer-based paradigm for routing state, then re-use React-Redux's @connect, instead of writing our own optimized subscriber.

[taion]

For that to work in a way that integrates sanely with Redux, we'd need to make router state serializable somehow, though, which is going to be tricky with async routes.

[ryanflorence]

Yep, been thinking about that ever since redux showed up, feel free to experiment.

But before we change the implementation of everything, maybe we can just iterate link in the right direction?

[taion]

The one caveat is that we need to be able to opt out from this behavior to support cases like preserving link active state during animations.

It'd be nice if we could clone router state in some way to properly freeze state in subtrees, but as currently implemented, we'd need something like the old <StaticContainer> hack to preserve link active state on transitioning-out routes.

[ryanflorence]

Might be a hack, but <Link ignoreChanges={animating}/> could work, you'll know you're animating, so it doesn't require extra finesse on the app's part.

It's hard to imagine any other use-case than animation, and a quick prop hack will work w/o requiring a bunch of changes for this one thing.

[taion]

For that to work, link components would need state. Might not be the worst thing ever.

[taion]

^- sarcasm – I think per-link component state for tracking whether they're active would be a very good solution for animations, that lets us get around the ugly <StaticContainer> hack.

[geekyme]

The way i'm solving this right now is to create a HOC around <Link />:

import React, { PropTypes, Component } from 'react';
import { Link } from 'react-router';

/**
 * Forces <Link /> to rerender itself on route change
 * This is not officially supported on react-router so we compose this behavior in a HOC
 *
**/
class ActivatedLink extends Component {
    static contextTypes = {
        history: PropTypes.object.isRequired
    }

    componentDidMount() {
        this.unlisten = this.context.history.listen(() => {
            this.forceUpdate();
        });
    }

    componentWillUnmount() {
        this.unlisten();
        this.unlisten = null;
    }

    render() {
        return <Link { ...this.props } />;
    }
}

export default ActivatedLink;

I think its probably better to keep this behavior out of the core and document some patterns to solve this problem as not everyone wants this behavior.

[taion]

That is not the correct implementation – you don't want to always re-render the <Link>.

[geekyme]

Yeah well, I suppose that is an optimization we can make in the callback. I believe RR 2.0 and the latest version of history probably provides some information to do this better?

[taion]

No, you need to track the actual rendered status of the <Link> itself.

[Narigo]

What would be a better implementation for this HOC then, @taion ? I was thinking of doing the same as I got into the similar situation that Link in my sub navigations did not update. Not really sure what you mean by "track the actual rendered status" and what's wrong with just forcing the Link to re-render.

[taion]

I mean that if we do release something to do this, it will only re-render the link when strictly necessary. It should not re-render the link if it's already showing the correct active state.

This is a little bit like what React Redux containers do, but with a bit more semantic knowledge.

[gaearon]

Since this is affecting a ton of our users in Redux, I’m probably going to start recommending this workaround: https://gist.github.com/gaearon/dfb80954a524709bcaf3bc584d9db11f. Yes, technically it could be faster but I’m not convinced that spending a few milliseconds is a big issue in real apps, unlike this being completely broken. In any case once you have a custom <Link> you can add ignoreAnimations or similar to it, and potentially improve the perf of this workaround. I’m sorry I don’t have the time or the context to contribute to this more meaningfully but I hope somebody finds it helpful.

---

Edit: now that I posted this, I see that it’s almost identical to @geekyme’s solution. I didn’t read into it carefully because I thought it uses a HOC (per the comment), but it actually just provides a component, like mine. Anyway.. no big difference. Either works!

[taion]

@gaearon

That actually won't do the right thing with async routes – router.listen is the forwarded history.listen, and will fire immediately on a history location update, rather than when the router's state changes. It might also flash in a weird way on redirects (because the history location will change twice, while the router location will only change once).

We don't currently expose a non-deprecated imperative way to listen to the router state, to the best of my knowledge (calling listen on the transition manager was previously possible, but would do awful things). Would need to add a router middleware to do that.

[gaearon]

We don't currently expose a non-deprecated imperative way to listen to the router state

API-wise, shouldn’t this.context.router.listen() do this? I would expect that it abstracts away the history from me and knows about the component states since I get it from the context managed by a component.

[taion]

I agree that the perf caveat is no big deal, though – isActive runs a route match, so if you have hundreds of links with active status that don't hit the index link optimization, performance is likely going to be so miserable to start with that any extra burden from re-computing active state is not going to matter much.

[taion]

@gaearon

I thought it did that before @timdorr corrected me: https://github.com/reactjs/react-router/pull/3340#discussion_r60081055

I don't know if we need router.listen to do that, though, if we can just set up a router middleware with componentDidUpdate. It makes the lifecycle-y bits more explicit.

[gaearon]

I’m currently creating a new course about Redux. I wanted to add some React Router in it but I feel bad recommending something that just doesn’t work unless you turn off all performance optimizations.

“Let’s make this app a bit more real world, so we add a router, and yeah, you have to turn off performance optimizations that Redux gives you in pretty much any container component.” This just doesn’t make sense to teach. 😞

I was hoping to have at least some kind of workaround because I don’t understand how to use <Link>s in the course.

[taion]

Scratch my statement about flashing on redirects – that won't happen.

I think what you have is okay as long as the user isn't using async routes. With sync routes it should be mostly fine...

Worth noting that for transitions triggered by links, that callback will fire while a React event is being handled, while for e.g. pressing the back button, it won't. Not sure if that matters.

The router middleware isn't hard, but probably using a router middleware doesn't make sense for a Redux course?

[gaearon]

The router middleware isn't hard, but probably using a router middleware doesn't make sense for a Redux course?

Yeah, to be honest it’s too many concepts to unpack just to get some very basic functionality. That’s implementation details leaking into the API 😄 .

[gaearon]

@taion @ryanflorence @mjackson

Would you be satisfied with a solution that:

• Puts something like _listenToTransitionManager on this.context.router.
• These callbacks are fired just before just before its state is updated.
• Adds an opt-in <Link subscribed> prop that switches it to use _listenToTransitionManager internally. This won’t work for animations, but since it’s opt-in, the burden of opting in is on the user, and they can opt out during an animation. Effectively the inverse of what @ryanflorence suggested in https://github.com/reactjs/react-router/issues/470#issuecomment-195573152.

[gaearon]

(We can also flip the default behavior in 3.0 as a breaking change and add an opt-in prop ignoreChanges for the animation use case.)

[taion]

This might be paranoia speaking, but I'm a little worried about _listenToTransitionManager because I feel like some overly-clever person is going to wire up an action creator to that, put params in a Redux store or something, then end up with really weird bugs when using Redux dev tools.

I don't know – part of me finds it weird that we have to jump through hoops to get context to work, while another part of me is hitting this exact same problem with Relay containers because they also implement shouldComponentUpdate.

This is just so messy. I think the "right" solution is to figure out a way to put router state in the store, add an isActive(routerState, linkTo), then have a <ReduxLink> that calculates active state from data in the store, but that's a long way away (and I can't justify spending much effort on this because I don't use Redux).

[taion]

This is probably going to get a lot _more_ confusing if/when we do https://github.com/reactjs/react-router/issues/3325 and put things like params, location, and route on context.router.

[gaearon]

This might be paranoia speaking, but I'm a little worried about _listenToTransitionManager because I feel like some overly-clever person is going to wire up an action creator to that, put params in a Redux store or something, then end up with really weird bugs when using Redux dev tools.

That would be their problem, really. Right now we’re comparing _potential breakage with devtools if somebody maybe creates some package_ with actual breakage in apps every day and beginners scratching their heads feeling JavaScript fatigue. 😄

Longer term, it’s fine to kill listen(), sure. But we’re not there yet and I can’t spend this effort either. I just need to unbreak my users. This makes sense to me: https://github.com/reactjs/react-router/pull/3340#discussion_r60169008. If you’re on board with this change, I can try to make it happen. (fixing this caveat as well).

[taion]

Brainstorming here... what do you think of building this in a generic way in a separate react-context-listener (please find a better name) library, then using that here (maybe plug it into <RouterContext>)?

I guess part of my discomfort here is dealing with this explicitly in the router per se when this is something that comes up with any user of context, alongside optimized containers.

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