React-router: Add onChange on <Route>

[taion]

Continuing from https://github.com/rackt/react-router/issues/2332 and also potentially relevant to https://github.com/rackt/react-router/issues/2546.

https://github.com/rackt/react-router/issues/2122 originally noted that there was no way to observe changes to query params. We attempted a fix with https://github.com/rackt/react-router/pull/2137, but it was incorrect and had to be reverted prior to 1.0.0.

To implement this correctly, we'd need to add an onChange (or equivalent) hook on <Route> that gets called when something about the <Route> changes, to allow users to respond to things like query or URL parameter changes.

[jimbolla]

I could use this make my routes less coupled to my authentication logic. What I have now looks like this:

    const currentUser = // ...

    const authorize = (authConfig) => (nextState, replaceState) => {
        if (!currentUser)
            replaceState(null, "/SignIn");

        if (!currentUser.isAuthorized(authConfig))
            replaceState(null, "/SignIn");
    };

    const routes = {
        path:        "/",
        childRoutes: [
            {
                path:        "abc",
                component:   ABC,
                onEnter:     authorize({roles: [a, b, c]}),
                childRoutes: [
                    {
                        path:      "admin",
                        component: ABCAdmin,
                        onEnter:   authorize({roles: [c]})
                    }
                ]
            },
            {
                path:      "xyz",
                component: XYZ,
                onEnter:   authorize({roles: [x, y, x]})
            },
            {
                path:      "foobar",
                component: FooBar,
                onEnter:   authorize({roles: [foo, bar]})
            },
        ]
    }

And with onChange it could look more like:

    const currentUser = // ...

    const authorize = (nextState, replaceState) => {
        if (!currentUser)
            replaceState(null, "/SignIn");

        let index = nextState.routes.length;
        while (index--) {
            const authConfig = nextState.routes[index].authorize || {};
            if (!currentUser.isAuthorized(authConfig))
                replaceState(null, "/SignIn");
        }
    };

    const routes = {
        path:        "/",
        onChange:    authorize,
        childRoutes: [
            {
                path:        "abc",
                component:   ABC,
                authorize:   {roles: [a, b, c]},
                childRoutes: [
                    {
                        path:      "admin",
                        component: ABCAdmin,
                        authorize: {roles: [c]}
                    }
                ]
            },
            {
                path:      "xyz",
                component: XYZ,
                authorize: {roles: [x, y, x]}
            },
            {
                path:      "foobar",
                component: FooBar,
                authorize: {roles: [foo, bar]}
            },
        ]
    }

My childRoutes would no longer be coupled to the authorize method, instead just declaring an authorize property. Custom route properties would also need to be supported to make this happen, if they're not already.

Edit: Confirmed that putting custom props on the route object are indeed available from nextState.routes, at least for plain route objects.

[mjackson]

I think I'd prefer to follow React's paradigm of just giving you new props whenever stuff changes. <Route onChange> feels like a workaround.

[taion]

I think that works for most use cases. The problem comes up for something like an auth flow -

If I'm going from e.g. /permissioned-items/foo to /permissioned-items/bar, it would be nice if there were a way to block that transition (and perhaps redirect away), using something like the onEnter hooks we currently use for auth.

As it stands, that's not possible; it would be nice if it were.

[mjackson]

Can you show some code to help me understand how onChange helps you do
something you can't do with props change?

On Fri, Nov 13, 2015 at 2:33 PM Jimmy Jia [email protected] wrote:

I think that works for most use cases. The problem comes up for something
like an auth flow -

If I'm going from e.g. /permissioned-items/foo to /permissioned-items/bar,
it would be nice if there were a way to block that transition (and perhaps
redirect away), using something like the onEnter hooks we currently use
for auth.

As it stands, that's not possible; it would be nice if it were.

—
Reply to this email directly or view it on GitHub
https://github.com/rackt/react-router/issues/2547#issuecomment-156578401
.

[taion]

Contrived example:

function authPage(nextState, replaceState) {
  if (!auth.mayViewPage(nextState.params.pageId)) {
    // In practice maybe this hits a remote and is async.
    replaceState(null, '/jail')
  }
}

const secureRoute = (
  <Route
    path="/pages/:pageId"
    onEnter={authPage}
    onChange={authPage}
  />
)

The reason you'd want to do this is because if you try to check this in componentWillReceiveProps, it's too late to prevent the route component from rendering at least once with the newly received props, which might be illegal in some way.

[beeant]

@taion yes I need something like this (onChange) for query string change

[taion]

@beeant Can you describe your specific use case and why it needs to be handled with a route hook rather than e.g. componentWillReceiveProps?

[beeant]

@taion The structure of my codes using redux following various example repos on github according to my needs and preferences. So I followed the way of loading async data using decorator like in https://github.com/coodoo/react-redux-isomorphic-example/blob/master/js/utils/FetchDataDecorator.js

It works fine, but it just doesn't work for my filtering feature.
I have a "list" page with search/filter function. The search query is passed through query string using something like:

this.history.pushState(null, "/search", {filters:filters});

expecting the onEnter will be re-executed with new search query when there is a change on the query string, but it doesn't.

I'm currently upgrading my whole app from react 0.13.x and react-router non version 1 → react 0.14.x and react-router 1.x, The Router.run used to be called on query string change.

[jimbolla]

It's not that it _can't_ be done with a component's componentWillReceiveProps, but whether it should. I don't like that I'm adding a new toplevel component to my route tree just to handle auth. There's also a significant difference in the amount of code.

What I have now is an entire component for auth called Authorizer:

const Authorizer = React.createClass({
    propTypes: {
        children: PropTypes.node.isRequired,
        routes:   PropTypes.array.isRequired,
    },

    contextTypes,

    componentDidMount() { this.checkAuthorization(this.props.routes); },
    componentWillReceiveProps(nextProps) { this.checkAuthorization(nextProps.routes); },

    checkAuthorization(routes) {
        if (!routesAreAuthorized(this.context.user, routes))
            this.context.history.replaceState(null, "/SignIn", {ReturnUrl: window.location.href});
    },

    render() {
        if (!routesAreAuthorized(this.context.user, this.props.routes))
            return null;

        return (
            <span>
                {this.props.children}
            </span>
        );
    }
});

// then in App.render...

render() {
    const routes = {
        component:  Authorizer,
        indexRoute: {
            component:   Layout,
            indexRoute:  {component: Home},
            childRoutes: [/* ... */]
        }
    };
    return <Router routes={routes}/>;
}

Whereas if we had onChange, I would replace Authorizer with:

render() {
    const user = this.state.user;
    const authorize = (nextState, replaceState) => {
        if (!routesAreAuthorized(user, nextState.routes))
            replaceState(null, "/SignIn", {ReturnUrl: window.location.href});
    };

    const routes = {
        component:   Layout,
        onEnter:     authorize,
        onChange:    authorize,
        indexRoute:  {component: Home},
        childRoutes: [/* ... */]
    };

    return <Router routes={routes}/>;
}

The latter example is much clearer since I can't avoid extra component hackery.

[taion]

@mjackson

I think @jimbolla's case is sort of the clearest illustration for why this helps. It's similar to the one I outlined in https://github.com/rackt/react-router/issues/2547#issuecomment-156585045 and shows the current next-best alternative.

It's nice to be able to just handle these kinds of things in route matching for changing a route's parameters the way we do for entering a new route, since it lets us save a lot of code in user space for blocking rendering of e.g. permissioned views that should not be seen.

I wonder if onChange should also receive the previous state, though.

[beeant]

I found a work around for my problem using componentWillReceiveProps but it's just a work around. I have to take out the promise from the fetchData decorator, and call that promise in the componentWillReceiveProps inside an if statement

componentWillReceiveProps() {
  if (nextProps.location.search !== this.props.location.search) {
     fetchDataPromise(nextProps.location.query, nextProps.params, this.props.dispatch);
  }
}

[ryanflorence]

I think I need to spend some more time with this but it seems to me onEnter and componentWillReceiveProps is all you need.

Is the problem that onEnter doesn't get called when only the params change?

/foo/123 -> /foo/345 ?

[taion]

The idea would be to deal with query changes, but given that we're currently discouraging data fetching in onEnter hooks, and redirecting on a query change would be odd, I think it makes sense to drop this issue for now.

[jquense]

I'd really like to see something like this hook if only for better clarification and organization in our app. componentWillReceiveProps technically does accomplish what we need but we are finding that its use in that way is getting somewhat unmanagable and messy, in a way that I think an onChange hook wouldn't.

First, its confusing to have to define route specific setup/param change/teardown logic in more than one place. It makes it hard to know where to look for certain logic, and where to place that logic in the first place. Having all the setup and tear down in onEnter and onLeave and then having all update code in the component handler itself.

Second (and related to 1) when a route renders more than one component there isn't always a clear, canonical, component to place responses to param changes. For instance if a page is rendering a Content and a Toolbar component, which one should respond to route param changes? This is especially true in a flux environment, such components are listening to same state, but don't share a common parent component that could listen an propagate that state. Even more so when some app state needs to change in response to the param change, but isn't otherwise related to any particular component.

It would be great to complete the API symmetry here and I think it isn't too much of a stretch right? onEnter and onLeave can also be defined in terms of Handler lifecycle methods, but there is clearly value in having them on the Route itself, for us at least the same is true for route changes.

[kadmil]

Hello, could someone tell me (or point to part of discussion), why data fetching with router hooks is discouraged?

In my case it's very convenient to have 'dumb' component tree, which just handles data from props and doesn't require custom-made query params change handling decorators or higher-order components calling some props-propagated function on componentWillReceiveProps only if router params changed, which is current alternative (as in code example at the beginning of the thread). As for now, having one type of hook to be called on entering route or query change would help a lot with avoiding much boilerplate code and unnecessary function-passing juggling.

[timdorr]

Because they won't fire when navigating between the same nodes in the route tree.

I achieve this with some decorator functions. You can also use something like async-props. Data fetching code should be colocated with the component that needs it. Putting it in the routes seems like a bad pattern to me.

[kadmil]

@timdorr I'm putting hooks only on leaf nodes, which change every time. Moving data fetching from components here means the very need of decorators/higher order components. That's what I could happily avoid with 'onChange' hook.

Another thing, looks like I'm not getting 'they won't fire' part — from which routes are we going to navigate to prevent hooks to fire, especially onChange ones?

[timdorr]

<Route path='users'>
  <Route path=':id' component={UserPage} onEnter={dataFetcher} />
</Route>

Going from /users/1 to /users/2 will not replace the route components, only update their props. And dataFetcher won't be called after loading up the first user's data.

However, if we add an onChange, that would fire during this navigation. I still think you should not have your data fetching outside of your component. It's very hard to see what needs what. Relay colocates data queries for this exact reason.

[jquense]

How do we feel about this hook? would a PR be a good way to move the discussion forward here?

[taion]

We consider data fetching in route change hooks to be an anti-pattern because it makes it difficult to provide feedback to the user that there is an asynchronous operation currently in progress.

[taion]

I'm personally on board with adding an onChange hook (otherwise why would I have opened this issue in the first place? :stuck_out_tongue:) but I can't speak for @ryanflorence or @mjackson.

[kadmil]

@timdorr I see, thank you!
As from my point of view (and mainly Redux experience) handling route changes from inside of the router would work far better than direct react lifecycle hooks. I'd say that's the reason I'm asking for your architecture/design point of view. ))

[nfcampos]

@timdorr using an onChange/onEnter hook for data fetching does not prevent you from locating data queries next to the component:
router.js:

import Yay from './yay'

<Router ... >
  <Route to='/yay' component={Yay} onEnter={refresh} />
</Router>

const refresh = nextState => {
  // routes is an array of all the nested routes nextState maps to
  // for now only leaf components use refresh
  // so i just get the component of the innermost route
  // tail = arr => arr[arr.length - 1]
  const component = tail(nextState.routes).component

  // route components define a static method called fetch
  // that initiates the api requests they need for render
  component.fetch && component.fetch(nextState)

  return nextState
}

yay.js:

const Yay = (props) => <div ... />

Yay.fetch = ({params}) => { ... }

export default Yay

(ps. this has some issues (eg. if you want to use this with some component which is not a top-level route component, this is assuming you inject the fetched data into the component in some other way) so i wouldn't actually recommend anyone use this)

(@taion it's not too difficult to have that refresh hook turn a spinner on and off as the request starts and ends to show feedback to the user (assuming you save the spinner state in a redux store or something like that))

[tybro0103]

Personally I'm moving away from react-router because of this very issue - the lack of an onChange. I believe there are a number of things that are routing concerns that just do not belong in a component.

Chief among them is auth/permissions checks. What if the user has permission to view /profiles/a but not /profiles/b? This would be some ugly logic to perform in componentWillReceiveProps because you’d have to figure out which props changed and do different things for different changes. More importantly, a view/component should not have to care about who’s looking at it - it should focus on rendering.

I also believe that routes/controllers _should_ be responsible for data loading. It’s _not_ hard to see what needs what because components can easily specify their data needs without actually fetching it themselves. With Relay this means having a query defined in the component, not executed by the component. For the rest of us it’s using propTypes to specify our data needs. I believe components are great for displaying data, but not fetching it. This is especially true on the server where you do not have a choice - data must be loaded before rendering.

Another big one is redirects. Should a component be responsible for looking at the data it’s been given to render and decide instead to trigger a redirect? It’s certainly possible, but again, it’s very ugly and it feels like a routing concern.

I know I’m going against the flow here, and perhaps I’m way off-base. If that’s the case, please set me straight. But so far I haven’t seen any valid arguments against this.

[taion]

Data fetching belongs in a middle layer – see async-props or react-router-relay. Like I've said upthread, we consider it an anti-pattern from a UX perspective to block route transitions in a modern SPA on async data fetches.

[nfcampos]

Imagine that you have your route components connected to the redux store with react-redux. And they get their data from a cache saved to the store. All that's missing is some hook to fetch the data initially and save it to the cache in the store. react-redux will then inject into the component when it appears in the cache. This hook that primes the cache can either run on the route's onChange or onEnter or on the component's componentWillReceiveProps and componentWillMount. But it doesn't block the transition, route components should know to show some loading state while they don't have all the props they need.

The only anti pattern here is you might end up having to declare your data needs for a component twice (once to select from the store, once for the fetch hook), but it's not too hard to avoid that I think.

[tybro0103]

@taion UX really has nothing to do with this. Putting routing concerns in the router does not mean that you have to wait until it has finished executing to render.

A common UX (preferred for some) is to simply display a loading indicator until the data is loaded and then render the view.

[tybro0103]

@nfcampos Not sure if I entirely understand what you're saying.

There's more to it than just data fetching. There's also performing checks (auth/permissions/etc) and redirects.

Also your data isn't always going to be cached...really not sure if that's relevant or not to what you're saying.

[taion]

That's insufficient – in general with that approach, the transitioning-out component would need to own rendering the loading indicator for the data for the transitioning-in component, which is silly.

[timdorr]

@tybro0103 The router will not re-render the route tree until the hooks have finished calling. It has to wait for them to execute for it to determine if it should even be rendering that path. The hook can change the path, so you would end up rendering the wrong component until the path updates.

[tybro0103]

@taion no it wouldn't. The router would. It would cause some data to be passed down to the component that it's in a loading state. In the case of Redux, you trigger an action that causes an isLoading flag to be set in the store and thus rendered. But this is a tangent really...

[taion]

The component that is logically in the loading state with respect to the fetched data is the component you transition _to_, not the component you transition _from_. However, as @timdorr says above, the router won't render the new component until after all routing hooks have executed. That's why we discourage using onEnter for async data loading.

[tybro0103]

@timdorr The hooks can be sync or async depending on if you provide a done arg or not. Regardless, even if it takes the hook a while to finish, you could still immediately trigger render updates to display a loading indicator by changing the data in your store.

[nfcampos]

But data fetching is asynchronous (in my apps at least). So the transition hook returns immediately, doesn't block the transition, thus while the data is being fetched it is the transitioning-in component that is rendered, thus making it responsible for rendering the loading indicator for its own data (as it should be obviously).

Nuno Campos

On 19 Feb 2016, at 21:23, Jimmy Jia [email protected] wrote:

That's insufficient – in general with that approach, the transitioning-out component would need to own rendering the loading indicator for the data for the transitioning-in component, which is silly.

—
Reply to this email directly or view it on GitHub.

[tybro0103]

@taion which component is in the loading state is irrelevant. Put whatever you need to in your store for your views to figure out where to display the loading indicator. The real issue here is performing routing concerns inside a view.

[tybro0103]

@nfcampos that's a solid approach. I think it can be achieved with either onChange or componentWillReceiveProps.

[nfcampos]

@tybro0103 my point is just that using the router hooks for triggering data fetching is not an anti pattern, obviously there're more uses for the hooks. I usually have more than hook on the same route (all synchronous) one to trigger fetching, one to check auth, etc

[timdorr]

@tybro0103 This breaks down for SSR. When on the server, you have to resolve those data fetches synchronously. If you put them in your transition hooks but don't provide a done(), they won't ever resolve. Instead, you'll be sending up your loading state to the client and negating the entire purpose of universal rendering.

I have one auth hook, but I first pre-fetch my auth data before continuing with any rendering (and the app can render that auth data however it likes). The rest of my data loading is defined via decorators/HoCs on my route components and sub-components.

[nfcampos]

@tybro0103 yeah I've been using a mix of onEnter and componentWillReceiveProps (because there's no onChange)

[nfcampos]

@timdorr my server side data fetching is synchronous so I don't have that problem (for now)

[timdorr]

By the way, I recommend looking into redux-async-connect for this kind of thing. It's basically like async-props with redux integration. Makes it very easy to data fetch inline with your components, rather than some external location.

[tybro0103]

@timdorr Not if you do it right. If you do indeed want to render the toView before its data is ready on client but not on the server, then you'd have to do a check in the route to see if you're on the client or server to know when to call done(). You might say performing that check is a bad thing because you want as little as possible to be different about the client and server. Personally, for that same reason, I would always wait until my router has finished before rendering the new view.

This could manifest a couple different ways UX wise. First, you could display the loading indicator in a common parent component. Second, you could change the thing that triggered the new route (link/button/etc) to itself turn into a loading indicator.

Github itself uses both of these approaches. Click from code to issues to see the former. Click on a file name to view its contents to see the latter.

[tybro0103]

@timdorr I'll check said libraries out.

Still though, it seems like routing/controller concern. That's a rather common pattern for routers/controllers... load the data and then render the view.

Also, again, there's more to this than data loading.

[nfcampos]

Thanks for the link! It looks like that does what I've been doing with the route hooks, I'm still looking for an approach to data fetching I really like

[tybro0103]

FWIW, I promise I'm not trolling. I've been pondering all this for some time now. It definitely seems to be a controversial issue. React Router has built a great community around it. I'd prefer to take part in it as opposed to rolling my own.

[timdorr]

@tybro0103 I don't think you are. If we can't take constructive criticism and use it to build a better library, then this isn't something that should be used. React Router isn't the only one under the sun (you might be interested in router5), but I think it's the best option for me and I'm here to help make it the best option for as many people as possible.

As for data fetching, I think the contextual relationships are important. Establishing those relationships declaratively is important, as you don't have to constantly define how that data is obtained, just that it is requested somehow. Your server might go to a database and your client might hit a REST API. You shouldn't have to write SQL and construct URLs everywhere in your application. Just fetchFoo().

A router actually has nothing to do with data fetching. It's responsibility is arranging what components (or controllers or widgets or whatever you call them) on screen at a particular time. It doesn't care what those components do and how they do it. Having it concern itself with those things overloads it's purpose and couples your route structure to your component behavior. That will have a detrimental effect on your application as it grows, as you'll have very un-DRY code that is resistant to change.

Your overall goal should be to have many tiny components and functions that do very little, but compose together in many different and interesting ways.

_That being said_, I do think onChange is a good thing. You may need to revaluate auth on a param change, which won't trigger from onEnter. I can see use cases for it. I just don't think it's a replacement for React lifecycle methods, which have worked just fine for, well, years.

[nfcampos]

It doesn't care what those components do and how they do it. Having it concern itself with those things overloads it's purpose and couples your route structure to your component behavior.

@timdorr i agree 100% that coupling routes with the components they render should be avoided, but doesn't the current api of the router encourage exactly that? components rendered by a route get the location and params in the props, all children components only get the router functions in the context. so any component you write for a route is forever destined to be a route top-level component (because only used that way will it have the location and params props it expects). so all these components are already tied to the router irrespective of using onEnter/onChange hooks, no?

[timdorr]

@nfcampos Props are a fundamental thing in React and are the primary way of passing state into a component. But no component should be concerned with _where_ those props came from, just that they exist (and, optionally, they match PropTypes). Any React component you grab from npm almost always starts off listing the props it takes in. That is the number 1 API of any React component. At its core, it's the embodiment of Tell, Don't Ask. The only difference is you're not doing the telling in this case, the router is. So, it's easy to look at it the wrong way. But just because this library is invoking your components (which is basically backwards from almost every other component out there), doesn't change the fundamental nature of props. They are not tied to your router at all. You can include a route component anywhere and provide your own props.params to them. They should still be able to function without you having to intuit that it needs some data fetched.

[kadmil]

@timdorr speaking of which, my Redux-inspired experience says it's good to have _dumb_ and _functional_ components most of the time, which translates to 'the less lifecycle hooks are involved the simpler app is'.
Alternative to router hooks here is having quite cluttered kind of component decorators, which will listen to componentWillReceiveProps and evaluate whether route has changed or not, which is far more intuitive to await from router part of the app.
Of course, another side of such allocation lies in understanding of undercover things like transition hang until synchronous code in the hooks is running, but we have to know something about things anyway.

I think we're running circles. It's about decisions, and I think all parties already had shown their cases and understand tradeoffs.

What do we need to have a decision for the issue?

[tybro0103]

It seems a number of us are on different pages as to what a router should even do.

I believe a router should respond to route changes. Which there are exactly two (non-error) valid responses to a route change, both of which need to be elegantly handled - rendering views and redirecting.

@nfcampos says “coupling routes with the components they render should be avoided”… I think that’s exactly what different routes are intended for - rendering different views. Which means at some point in your app whether it’s the router itself or a top level component, the current path absolutely does have to coupled to a view.

@timdorr says “Its responsibility is arranging what components are on screen at a particular time”. I agree partly - that is part of its responsibility. The other part is deciding to redirect.

[tybro0103]

There are some irrelevant points being made here:

• “You shouldn't have to write SQL and construct URLs everywhere in your application.”
• “Your overall goal should be to have many tiny components and functions that do very little, but compose together in many different and interesting ways.”

I agree 100%, and nothing I suggested would mean contradicting either of those.

[tybro0103]

Now one main point of disagreement is whose responsibility it is to load the data that a view needs to render.

First, there’s the part where you should be able to look a component and know what data it needs. as @timdorr puts it: “As for data fetching, I think the contextual relationships are important. Establishing those relationships declaratively is important”. I agree with that, but I can’t help but wonder… are you forgetting about propTypes? As you put it yourself: “Props are a fundamental thing in React and are the primary way of passing state into a component. But no component should be concerned with where those props came from, just that they exist” exactly - this is props/propTypes are for! Why not use them to declare your data needs?

One of the points against using the router to fetch data is that it would clutter up the router. That’s where some basic project organization comes into play. If you were to use routers to fetch data, that doesn’t mean you have to do it in a cluttered, unorganized way. You don’t even have to do it all in the same file. Even if the router triggers the data fetching for a view, it doesn’t have to be the thing actually hitting the db or making the ajax call. For my case I would use the router to trigger a single redux action which would fetch all the data it needs. It could even be the router that triggers a static fetchData method on a component if you really want to go that route (no pun intended).

As a side note, the organization aspect is actually another thing I don't like about react router - there's really not any outstanding way (it is possible) of breaking the router out into multiple files. Like Express for example - very flexible in organization/architecture.

One thing I’d like to point out is that using the router to fetch data is a very common paradigm that many developers are already used to. Look at Express, Rails, etc.

All that said, my number one reason to use routes to trigger data fetching is because of the other thing you have to do in routes - redirecting. Let’s contrive up an example… Let’s say the user can view /users/batman but does not have permission to view /users/superman. Let’s also say that the only way to learn this information is to make an http call to fetch the superman data and it responds with a 401. Also, you don’t make this call until you need to render the superman view. Now a user navigates to /users/superman; a call is made to fetch superman; it responds 401; you need to redirect.

So now you need to perform a redirect from inside a component as the result of going to a new route. Do you see the problem yet? On the client, it’s some rather ugly logic, and it’s happening in an odd place - the view (poor separation of concerns). On the server it’s even worse - it’s actually not even possible to respond with a redirect from a component unless you cause a side effect to happen. Which you _could_ do, but why? There’s a really perfect place that’s been around for quite some time to handle this - the router. Maybe just not… react router.

[tybro0103]

Now back to the problem at hand - onChange. I think both camps of thinking could accomplish what they want if this method were added.

It's been brought up before and it will continually be brought up again - because it's a deviation from how most other routers work. Personally, I could even see onEnter being triggered each time there's a change - it's how the browser itself treats it. But anyways, I'll happily take an onChange.

Even if we disagree on how applications should be architected, why actually prevent me from architecting the way I want to? I've made some points, and some disagree which is fine, but I think there's enough validity to my points to justify an onChange.

[taion]

Let's get back on topic, please.

We are building the router in such a way as to match the best practices we've picked up from extensive experience in building these applications, and we will give priority to practices that we consider to be good. You can choose to take advantage of this or not.

Regardless, discussion of architecture here is cluttering up the issue.

@ryanflorence, what do you think of @jquense's points in favor of adding onChange? Namely, in the case when a route has multiple components.

[tybro0103]

Yes, the topic.

I’ve made a point in favor of onChange (and thoroughly supported its architecture validity) that I have discovered through my own extensive experience building universal apps.

It boils down to: without onChange, you’re forced into doing things in the component that you should be able to do in the router. The easiest example of this is needing to redirect due to permissions when going from /users/a to /users/b.

I too would love to hear more opinions on the matter. Another vote to have @ryanflorence or @mjackson chime in.

[tybro0103]

@taion Clashing ideas does not make one or the other of us right or wrong. I think that thoroughly hashing out the differences between your own ideas and others that challenge them is how end products are going to be the best that they can be. Take-it-or-leave-it is a little harsh.

[jquense]

poking around at a PR now...I am not quite sure how to fit an onChange hook in with onEnter.

Since onEnter _is_ called on param changes there would be some overlap between the hooks, unless we make onChange only fire on query changes. To my mind the most obvious API is to parallel the lifecycle hooks, with enter == mount, change = wrp, leave = unmount. But there is no path to that without some sort of backwards incompatable change, which (I assume) disqualifies that approach.

Personally I don't find the overlap to be a problem conceptually, but maybe it doesn't make sense?

[taion]

Trying to convince us that a proposed feature makes an anti-pattern easier is not a good path toward getting that feature added.

@jquense

I think the most expedient way to go about this is to just fire onChange for the routes that don't change. I guess we can think of params as being like implicit keys or something.

This neatly mirrors how isActive works anyway (i.e. onChange would only fire when isActive would not change).

[jquense]

it also occurs to me that it could be called param changes and not overlap if the presence of an onChange handler is used as an opt-in for that behavior. It shouldn't break anyone since ther isn't a handler there now, but it does potentially require a refactor if you do want to add in an onChange...

Maybe I'll just stick with query changes for the time being and we can discuss it more in the PR

[taion]

Not just query changes but also child route changes too. The confusing thing is that param changes also lead to onLeave being called.

I feel like the straightforward way to do this is just to add that missing last category to computeChangedRoutes.

[tybro0103]

@taion I've made many points on the stance that it is in fact not an anti pattern, and that the real anti pattern is having the component fetch its own data rather than simply declaring its data needs. Instead of addressing these points you have resorted to saying you're experienced and assuming I am not.

[taion]

Yes, and that's exactly what both async-props and react-router-relay provide – but not in transition hooks, which is not where the actual logic should run.

[timdorr]

@tybro0103 That is most definitely an anti-pattern. We're not building a data fetching library, we're building a router. These hooks are only meant to affect the behavior of the router itself, not outside systems. But let's drop this, as that's not the point of this API. The point of this is to allow us to affect router behavior when routes simply change, rather than only when they come into view or are removed.

@taion & @jquense I think, given that a param change is consider a route leave-enter event (triggering any setRouteLeaveHooks, that onChange should be limited to just any non-route-changing event, i.e. query changes. What about other location changes, such as state? Maybe this should just be whenever the location is mutated but doesn't result in a route change (at that level of the tree)?

[taion]

I don't think we need the architectural discussion here – it seems obvious from an impl perspective to just run onChange on all the un-exited and un-entered routes here: https://github.com/reactjs/react-router/blob/v2.0.0/modules/createTransitionManager.js#L69-L97

Most straightforward implementation-wise, easiest to explain, &c.

[timdorr]

@jquense Is that the approach you've taken thus far? It sounds like we just need another return from computeChangedRoutes for unchangedRoutes or something like that. If not, I can take a swing at it.

[jquense]

Yeah I have more or less that implementation sitting here. minimal, straightforward. I ran out of time to send something today, but should finish it up tomorrow

[martindanielson]

Just adding my support for this enhancement.

I have a use case where we need to add something to the URL if it is not specified and another one where we want to clear things out from the URL.

It is just so much nicer to be able to do this while transitioning so that we can avoid the whole "load with wrong format, check a lot of things and then do a redirect" logic that otherwise is required.

I understand that this might go a little against principles of React but I believe it does not go against the principles of what a router is responsible of.

@taion, have to commend you for your willing to improve on this. Thank you for giving us mortals a voice :)

[esellin]

Until react-router implements onChange on a Route object, you can do this:

browserHistory.listen(function(ev) {
  console.log('listen', ev.pathname);
});

Credits to @DominicTobias
from https://github.com/reactjs/react-router/issues/964

[ryanflorence]

I'll +1 this addition. To me, onEnter is the only one that makes sense, and the only use-case it was designed for is redirecting.

We already have onLeave (which I don't understand what people use it for) so either get rid of onLeave or add onChange, but to be missing onChange is weird.

[taion]

I think that's the strongest argument – if we have onEnter and onLeave, we really should have onChange.