Add `listen` prop

[wmertens]

In v2.0.0-beta.7, routes no longer listen to history events. That works fine in many cases, but when intermediary components implement sCU, that means routes no longer apply.

This is easy to encounter when you e.g. implement a composable master/detail view with relative routes.

The official way to be updated when the location changes is through providing the location prop, but that is quite a lot of work: You need to get the location from the Router component, store it somewhere (e.g. Redux) and then retrieve it and pass it to the route as a prop.

It would be much simpler if instead you could just say <Route listen .../> and it would listen to history like before. Very little code, very low overhead, and once the context API becomes sane, it will no longer be necessary.

[pshrmn]

A listen prop does not help if the component rendered by the <Route> implements sCU (e.g. a connected component).

A better solution is an HOC or composable component that re-renders it's children on location changes.

// sCU would still block updates
const One = connect(...)(BaseOne)
<Route listen path='/one' component={One} />

// Placing the listener after sCU gets around the block
const ListenOne = connect(...)(listen(One))
<Route path='/one' component={ListenOne} />

[wmertens]

But, the rendered component gets the current location as one of the props?

Also, if that component implement sCU that's fine, it's the Route that decides which components should be rendered and what the components do is up to them.

Finally, adding a listen prop that implements previous behavior, simply implements the behavior that was already working, so that means it should just work…

On Sat, Mar 4, 2017 at 5:34 PM Paul Sherman notifications@github.com wrote:

A listen prop does not help if the component rendered by the implements sCU (e.g. a connected component).

A better solution is an HOC or composable component that re-renders it's children on location changes.

// sCU would still block updatesconst One = connect(...)(BaseOne) // Placing the listener after sCU gets around the blockconst ListenOne = connect(...)(listen(One))

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ReactTraining/react-router/issues/4629#issuecomment-284162831, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWlpYctvwA5qPnetDoWxEjZeFQOsOtks5riZKlgaJpZM4MTHK1 .

[pshrmn]

Do you have a code snippet that demonstrates the issue that you're running into? Perhaps I'm just not understanding correctly, but I really don't see where this would be useful.

[wmertens]

Okay, suppose you have Accordion which shows one section at a time based on Route. Then suppose you have a third-party component in petween that uses sCU.

Your app looks like

<Router>
  <Route ...>
     ...
     <ThirdParty>
       ...
       <Accordion sections=.../>
...close everything

Any history changes won't ever make it to Ąccordion… (on phone sorry for brevity)

[pshrmn]

Where does the <Route listen> come into play, though? Between <ThirdParty> and <Accordion>?

[wmertens]

Accordion uses Route internally to know the active section, so it needs <Route listen/> there.

[pshrmn]

I guess what I'm saying is that is that you would do something like:

<Router>
  <Route>
    <ThirdParty>
      <Listener>
        <Accordion>

The <Listener> would listen for navigation changes and call forceUpdate to re-render its children. That way, you would only need one listener inside of the sCU'd component instead of one for each <Route>. Also, adding <Route listen> wouldn't help any <Link>s and other location-aware components.

I was actually experimenting with subscribers behind sCU a bit and ran into some issues because of how React's context works (it is push not pull, so a component can't get a fresh copy of the context from its parent). I think that we will either need to use Object.assign again or have some sort of refresh method that allows a component to pull its parent's context.route).

[wmertens]

For routes, adding listen is very low overhead, because there are so few routes in an app. For links, that can be a lot more, but that is only needed in a few cases, nav links (only a few), and relative links, which could also be worked around by re-rendering with a different key.

The method you propose still requires putting <Listener/> inside any pure component. That could mean more Listeners than Routes.

[mjackson]

@wmertens Using listen everywhere is fighting with React. It's going against the React model.

Instead, in beta.7 we chose to listen in exactly one spot: the <Router>. Then, anyone who's using shouldComponentUpdate is responsible for passing the new location through to their "pure" components to let them know that something changed. Otherwise, if we subscribe in every <Route> then we get a lot more re-rendering than is necessary every time the location changes.

[wmertens]

@mjackson it may feel like fighting, but it is a direct consequence of the broken React context API.

The idea of context is that there are certain global values that are not changed frequently and that would be cumbersome to pass through the entire React app. That's a perfect fit for location and why you are using it. The brokenness is that context does not update all the components that use it when it changes due to sCU.

So, until this gets fixed (not until React 17 from what I understand), workarounds are needed.

You would like the workaround to be that people spend a lot of time hooking up a location subscription system, so that you don't have to support it.

I contend that a listen prop is an equally valid workaround, low overhead, very suitable in most cases and orthogonal to a location subscription system.

Both workaround will no longer be needed once the context API is sane.

Finally, your re-rendering point is invalid; wherever you subscribe to location updates is wherever you would use <Route listen .../>, and in both cases you get the same amount of re-rendering. I am specifically asking for a way to opt-in to subscribing so it does not happen in every route. Furthermore, if you put location in a Redux store, every connect will be evaluated whenever location changes. That is also a lot of function evaluation.

TL; DR: @connect(({location} )=> ({location})) and <Route listen .../> are roughly equivalent in performance but the latter is way nicer for devs.

On Sun, Mar 5, 2017 at 6:34 AM MICHAEL JACKSON notifications@github.com wrote:

@wmertens https://github.com/wmertens Using listen everywhere is fighting with React. It's going against the React model.

Instead, in beta.7 we chose to listen in exactly one spot: the . Then, anyone who's using shouldComponentUpdate is responsible for passing the new location through to their "pure" components to let them know that something changed. Otherwise, if we subscribe in every then we get a lot more re-rendering than is necessary every time the location changes.

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/ReactTraining/react-router/issues/4629#issuecomment-284202921, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWlqf6ojGGrkL4BG-K0kuXv_pQ9RmWks5rii67gaJpZM4MTHK1 .

[wmertens]

I wrote down my thoughts on RRv4 and the last paragraph has a link to a simple <Route/> wrapper with listen, but it would be more efficient if the listening were done in <Route/>.

[mjackson]

Yes, I realize that context is broken. But I disagree that it's our job to fix it with subscriptions.

[wmertens]

I fully agree it is not your job to fix it, but adding optional useful behavior with a very small footprint in the place where it can be done most efficiently seems to be the right thing to do for a framework?

The way I fix it in the gist I linked at the bottom of my post works, but adds 2 extra elements around Route :(

On Mon, Mar 6, 2017 at 2:21 AM MICHAEL JACKSON notifications@github.com wrote:

Yes, I realize that context is broken. But I disagree that it's our job to fix it with subscriptions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ReactTraining/react-router/issues/4629#issuecomment-284280796, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWlhM-jt14PnGoPIqulQryOQiQ459Hks5ri198gaJpZM4MTHK1 .

[agundermann]

Yes, I realize that context is broken. But I disagree that it's our job to fix it with subscriptions.

In that case I think the context API should be made public so that people implementing their own subscriptions don't have to worry about stuff breaking in patches/minor versions.

[pshrmn]

Subscriptions aren't going to work. Anyone trying to get around sCU is going to run into the same issue: the subscribed function will be re-rendering with the context from the previous render. To get around this, you have to maintain a single object that will be used within context and mutate it when the location changes. That is annoying and makes you ask the real question:

Why should we be using subscriptions to get around sCU?

If a component says "don't update me", we should respect that. Typically all you have to do to get sCU to return true is to pass a prop that changes on every location change (I.e., the location object). That should just leave components whose sCU always returns false. If that happens with components that actually should update, then they are poorly designed. That component should be fixed to accommodate prop/context changes so that it can be triggered to re-render. React Router shouldn't be expected to provide solutions to get around another piece of code's bad decisions.

[wmertens]

IMHO context is for global things that change "rarely", like language settings.

This implies that when you change context, it's ok to check the entire mounted tree to see which components should be updated, ignoring sCU. (sCU can be used to check if a component wants to update given a context, but its children should be updated too)

But maybe that's a discussion for https://github.com/facebook/react/issues/2517

On Mon, Mar 6, 2017 at 5:40 PM Paul Sherman notifications@github.com wrote:

Subscriptions aren't going to work. Anyone trying to get around sCU is going to run into the same issue: the subscribed function will be re-rendering with the context from the previous render. To get around this, you have to maintain a single object that will be used within context and mutate it when the location changes. That is annoying and makes you ask the real question:

Why should we be using subscriptions to get around sCU?

If a component says "don't update me", we should respect that. Typically all you have to do to get sCU to return true is to pass a prop that changes on every location change (I.e., the location object). That should just leave components whose sCU always returns false. If that happens with components that actually should update, then they are poorly designed. That component should be fixed to accommodate prop/context changes so that it can be triggered to re-render. React Router shouldn't be expected to provide solutions to get around another piece of code's bad decisions.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ReactTraining/react-router/issues/4629#issuecomment-284453865, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWlrRKlJV_z0nZw9t2sQAfoF-aAyOjks5rjDcNgaJpZM4MTHK1 .

[mjackson]

In that case I think the context API should be made public

Absolutely agree, @taurose. I've already got a page in the docs about context.history. If it's in the docs, it's public.

context.history is our imperative API. When we split context.router into context.history + context.route in 1f0107986f98b38e175819fb0eca77c42c18b6bb, the main motivation was to expose context.history unmodified from the object you passed to <Router history>. This gives you full control over the imperative API so you can push, replace, and even listen if you want. However, I'd recommend you don't use listen and instead just let React do its thing. If you don't, you're going to end up doing a lot of extra work to ensure your subscriptions don't cause a bunch of re-renders.

seems to be the right thing to do for a framework?

We're desperately trying to not be a framework in v4. From the v4 FAQ:

React Router was not a "React router", it was a routing framework for React. An accidental framework with APIs that were not only redundant with React, but incredibly difficult to build an ecosystem around.

You can think of React Router v4 as just a bunch of primitives, like React itself, except focused specifically on routing. It's basically history + path-to-regexp + a few React components to stitch things together in your render methods.

[matthewrobb]

Honestly I am having trouble following this discussion but 4.0.0-beta.7 is completely unusable for me right now. I don't expect anyone to try and figure out what my problems are I just thought I'd report my naive diagnosis:

Things work on initial load as I'd expect but changes to browser history seem to have no impact on my application (where previously it would have).

[wmertens]

@matthewrobb try wrapping RR with this gist: https://gist.github.com/wmertens/b581322d5212035ffe7e6b4fd6222647 (so just import from that file instead of from RR)

Then, for <Route .../>s that don't seem to update and that are deeper in your application, add the listen attribute <Route listen .../>

If that helps, it's because there are pure components in between doing their thing.

[matthewrobb]

@wmertens Question, does the render prop of Route get treated like a stateless functional component?

<Route render={props => (<div>...</div>)} />

[wmertens]

@mjackson I understand you don't want to be a framework, poor choice of word on my part. I applaud the new RR direction, it is really great.

However, it sounds like you want to provide a toolbox so a majority of devs will be served in their routing needs. I have a feeling that listen might be a low-cost and useful addition to that toolbox.

Reading your response, it also seems that you assume everybody would add listen to all their <Route/> elements. However, that is not necessary, except when you are below a sCU component. Furthermore, when re-rendering a route will result in a tree with the same children and props, would that same sCU not prevent the re-rendering?

I am using listen where needed in a quite complex app, and at any one time I only have maximum 2 Routes that are listening (a ListDetail component inside an Accordion component inside a Redux Form that has very strict sCU).

[wmertens]

@matthewrobb no, it gets run every time the <Route> is rendered; there is no sCU. The things you render with it can have sCU though.

[matthewrobb]

@wmertens Okay. I see how this is going. Thank you!

[matthewrobb]

This is the simple break down of my case, what wasn't working and what is working.

Doesn't Work:

class MyComponent extends Component {
  render() {
    return (
      <Router>
        <SomeProvider> // stateful component but wrapped in a HoC
          <Route ... />
        </SomeProvider>
      </Router>
    );
  }
}

Does Work:

class MyComponent extends Component {
  render() {
    return (
      <Router>
        <Route render={props => (
          <SomeProvider {...props}>
            <Route ... />
          </SomeProvider>
         )}/>
      </Router>
    );
  }
}

[matthewrobb]

I'll be honest, I see the rationale for this change but it makes me question the existence and/or need of having a top-level component if I don't get the benefit of a deeply piercing route sync. I can't always control what sCU behavior is happening inside of some other component but if my <Route/> is written in the same JSX as the <Router/> then I'd wager 99% of people's intuition is going to be it should "just work".

[mjackson]

@matthewrobb Propagating location changes to deeply nested components is only one job of the <Router>. The other is to set context.history so you can navigate (i.e. push and replace) in those components.

I think all of this really just shines a light on how context is broken, which, to be fair, the React authors have cautioned us all about using.

[matthewrobb]

@mjackson I don't disagree with you at all. It really is a tough problem. I'd argue that context is wholly broken for anything OTHER THAN rule-breaking magic.

This conversation just leads me to want to have an imperative Router that is treated like any other data store. If it should be passed down than let users do so using a <Provider/>. Right now it seems like <Router/> is a glorified private state holder and (via context) distributor. But there exists a stronger convention for this already: modules.

[mjackson]

Absolutely agree, @matthewrobb. This is why we already extracted out all our state and put it in history. That's our imperative API. The router does a bit of path-to-regexp matching, but that's all just state that anyone can derive from the current URL.

If you've got some ideas for a more useful component abstraction, I'm all ears. The components we've got seem to do their job well though.

[matthewrobb]

@mjackson It just seems like you could drop <Router/> altogether. Make it an imperative that most apps would create a singleton instance of export default new Router(...). And if someone wanted context propagation then force them to use their flavor of <Provider/> otherwise require that <Route/> and friends take router={routerStore}. 0 magic everything requires explicit attachment. No baked in reliance on context. Every single one of these more frameworky features could easily live in a react-router-app package or similar that meets somewhere closer to what people expect today.

It's late in EST where I am and my mind is fogging up so hopefully some of this made sense to someone!

[mjackson]

@matthewrobb We were using subscriptions before (your router prop is exactly that) but we took them out to avoid lots of unnecessary re-rendering. 😞

Anyway, I feel like I'm just repeating myself now so I'm going to close this issue.

[donaldpipowitch]

@mjackson I understand why you removed subscriptions as the default behaviour, but what are the arguments against offering something like LocationListener as another primitive of react-router? It seems to be a useful primitive for small components which are often a leaf node in the component tree. Something like <NavLink/>. It is probably quite safe to re-render it and more developer friendly, because one doesn't need to pass location down to something small like <NavLink/>, because some higher component used shouldComponentUpdate.