Add onChange on <Route>

[taion]

Continuing from #2332 and also potentially relevant to #2546.

#2122 originally noted that there was no way to observe changes to query params. We attempted a fix with #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
#2547 (comment)
.

[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 #2547 (comment) 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.

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