[added] onChange route hook

[jquense]

closes #2547

Adds an onChange hook for Routes, which allows routes to respond to non leave/enter changes.

I changed up the signature a bit, which may be more self serving than anything else, but I also think it nicely mirrors the React hooks. In particular onChange is called with prevState along with nextState and replace. Which just makes checking for the particular change you are interested in easier. It also allows for not doing any work in the case of a location not changing in a way that is relevant to the particular route. Let me know tho if I'm off base there. The diff is a bit noiser then it might otherwise be if I left that out

[taion]

asyncArity - 1? To avoid dumb crap.

[jquense]

good catch.

[taion]

BTW, we don't do changelog generation from commits, so I think we usually avoid [added] in commit subjects.

[taion]

Looks fine to me implementation-wise. @ryanflorence @mjackson thoughts on adding this? Does it make sense to pass in replace to an onChange hook?

[taion]

Ping @mjackson @ryanflorence

[ryanflorence]

I don't understand what the use-case is here.

@taion mentioned query changes somewhere else, but those come through Router.props.render and Router.props.onUpdate don't they?

Also, could make your own history along with userRouterHistory to do this, can't you?

What is the use-case?

[taion]

(from #3166 (comment))

Picking up on #3166 (comment), if we care about route transitions w/r/t data loading, then you may need to fetch new data on query changes, e.g. if one of the query fields corresponds to a filter on a list view or something.

[taion]

i.e. if I have an onEnter hook telling me that I've gone from /users/123 to /users/456, it seems like I should have a parallel hook to tell me that I've gone from /users?search=foo to /users?search=bar.

[ryanflorence]

Ah, yeah, I'm biased because I use the Router render prop instead of these hooks, but yeah, I see now if you're plugging into the route hooks you'll need an onChange to know when to load new data for query changes.

But I still think it's the wrong place for data integration, so it's hard for me to support more API when there's API designed specifically for this.

onEnter is to redirect, I have no idea what you'd ever do in onLeave but it's there anyway.

I'm 👎 unless somebody can show me what data integration in route hooks gives you over in the Router render prop. I don't care enough to block though.

@mjackson ?

[ryanflorence]

Okay ... so using the hooks for data integration lets you get the "what stuff changed?" for free from the router. I had to reach into lib/computeChangedRoutes for AsyncProps to get that, maybe we should be sending the leaveRoutes and enterRoutes as props to route components, then integrations don't have to compute the diff themselves.

Other than that (which is a big deal), I don't see the value in route hooks for data loading.

[taion]

Isn't that the case you're supporting for #3166 though? i.e. you bring up the example there of data loading from onEnter and onLeave hooks.

[taion]

BTW, a tangential but related thought. Consider the URL https://github.com/reactjs/non-existent-repo. Quite nicely, GitHub 404s on this and takes you to the actual 404 page.

Consider setting this up in React Router. Imagine your route config is something like:

<Route path="/" component={App}>
  <Route path=":org" component={Org}>
    <Route path=":repo" component={Repo} />
  </Route>
  <Route path="*" component={NotFound} />
</Route>

If you want to display a mostly full-page 404 on hitting a non-existent repo in an existing org, how would you do this? Ideally you'd want to localize this logic to the ":repo" route, so you'd need something like #3098.

But the next question is, how do you actually make this work between the client and the server? You don't want to round-trip to the server twice (once to check for existence, once to actually grab the rest of the data – though with e.g. Relay it might be hard to even avoid this pattern); this means you can't finish the matching unless you've already retrieved the data.

This pattern also wouldn't play nicely with nested routes with absolute paths (but I don't like that feature anyway).

Not directly related to this topic, but handling something like this seems like it'd be very tricky without something that looks like data fetching in onEnter (i.e. blocking the transition for a data fetch operation).

[jquense]

thanks for the responses y'all. questions about where data fetching is appropriate aside, my main motivation and use case is that the many routes we have that render two or more components which effectively makes the route the "common parent" which makes it the obvious place to fire off the common state stuff for the route.

things like Async Props don't help bc I'd have to over specify data needs in each route component. while It'd be nice to have relay like smarts to dedupe and optimize those calls, we aren't able to and rolling our own feels like overkill when the route hook can give us that for free.

[jquense]

which is to say I think avoiding hooks in the route made more sense when each route only rendered one component so it was easy to reuse react lifecycle hooks. Now that routes can specify multiple components it gets tricky knowing where the right place to locate that stuff

[andreigabreanu]

A more complex example where onChange makes a lot of sense is when you want to do something like this:

Page loading -> Load Locale if not already loaded (either by Route Param or doing some other logic like check query string or an API based on IP etc...) -> Load Auth if not already loaded -> Check If Allowed always (!) -> not allowed => redirect a& restart this flow; is allowed => Page loaded => Render component .

You can't do it with onEnter since it will only be run once. I guess you could come up with a way to do it via Components but it's much more complicated than just having these simple hooks.
If you have onChange there as well, then you can simply run these "middlewares" all the time and let the user decide the logic (i.e. if not changed, continue, else redirect or error, etc)

I would even go further and add an "onAny" (bad name but just to get an idea) where it runs the callback both on onEnter and onChange (think of it like a middleware pipe that always gets run when the router changes something).

Regarding the idea that I saw being repeated here a lot by @ryanflorence that its a bad UX/UI to block the transition, I don't agree - as long as you provide feedback to the user.
If you have a redux/flux store you can easily wrap the Router in a component that is hooked to a store and displays a page loader based on the store state. When a transition starts, you update the store state (it would do that on "onChange/onStart"), the user gets the page loader, the JS loads stuff then after some time, the callback is called and the components are rendered.
This is especially useful when you want to do some async check but not render the component before the check is done.

And it is useful because it is so easy to do it via onChange rather than rolling up your own HoC. So I am +1 * 1000 for this. I've used React Router in at least 7 projects so far and always ended up wrapping /hacking the Route and/or Router to provide a similar behaviour when it should really be in the library IMHO.

[taion]

Per #2547 (comment), looks like we have consensus that we should add this feature.

I think we should merge and document this feature and cut a 2.1.0.

[taion]

I'm going to go ahead and merge this.

[ayou33]

@taion dose it only work for browserHistory?

let routes = {
    path: '/',
    component: App,
    onChange () {
      console.log('hash changed')
    },
    childRoutes: [
      require('./modules/User')
    ]
}
<Provider store={store} key="provider">
  <Router
    history={hashHistory}
    routes={routes}
  />
</Provider>

nothing happened in onChange hook when i code like above, any help?
TKS!

[andreigabreanu]

@hubu The onUpdate doesn't seem to be merged in the latest NPM version (2.0.1) . I had to clone the repo locally and use the master branch in order to use it.

[bhoomit]

How do I use onChange for entire Router?

[andreigabreanu]

One possible option (from what I know) is to setup a master Route node with all your app routes and set the onChange/onEnter on it. This way it will run the hooks globally.

[bhoomit]

@andreigabreanu awesome, it worked 👍

[mjackson]

Gah, missed this. Sorry. I get too much email.

I'm generally 👎 on new API unless there's a solid use case. React gives you "on change" for free in componentWillReceiveProps. Adding onChange to routes is unnecessary API, IMO.

[ryanflorence]

Yeah, we should have killed onLeave instead.

[taion]

The motivation here FWIW was to have better support for the case when the user has named components, since cWRP is not quite as neat there, since you'd have to make an arbitrary choice of which component to stick the callback on.

[bhoomit]

@mjackson componentWillReceiveProps makes sense in case when I want to do something at individual component level. But in case when I want something to happen on every route (e.g. fire analytics event) onChange is necessary. Correct me if I'm wrong.