onEnter called on query changes

[arnif]

These are my routes:

<Route path=":username" component={UserDashboard} onEnter={::this.handleUserEnter}>
      <Route path="photos" component={UserPhotos} />
      <Route path="statistics" component={UserStatistics} />
</Route>

And in my app I have <Link to={${username}/photos} query={{p: photo.id}}>Photos</Link>

Whenever I click that link the onEnter function is called.

But navigating from /photos to /statistics does not call onEnter (which is expected since i'm not leaving the UserDashboard component).

I found this PR (#2137) and this change causes the problem, reverting fixes it.

[taion]

This looks like an intentional consequence of #2137.

@mjackson @knowbody Are we sure we want the behavior in #2137 though? It seems inconsistent to fire the hooks when changing query parameters, but not when changing e.g. which child route you're on.

[knowbody]

ideally we want both I think, @arnif can you submit the PR with a failing test?

[taion]

@knowbody Really? I thought it was intentional that e.g. in going from /foo/a to /foo/b, that we wouldn't trigger onEnter or onLeave handlers for /foo.

[knowbody]

hmmm... why not?

[taion]

It just seems weird in that case to run the onLeave handler for /foo, then run the onEnter handler for the same route immediately afterward.

This wouldn't e.g. be a good fit if we were using patterns like confirming navigation, since you're not actually leaving the /foo route.

Also I assumed it was intentional because otherwise why even bother computing changed routes? It'd be easier to just run onLeave on all old routes and onEnter on all new ones.

[knowbody]

you are right

[knowbody]

@taion should we revert #2137 then? and add a comment in #2122 ?

[taion]

I think that would be the cleanest thing to do, but I'm just trying to infer @mjackson's intentions from the code. I don't know what the actual intended design is.

[taion]

I think the problem is something like - we can't map query fields to specific routes the way we can URL parameters. It has to be all or none.

[taion]

@mjackson Any thoughts on what should happen in this case?

[sikanhe]

This should defintely be reverted, changing query param does not indicate entering a new "page" and it has undesired behavior such as scroll to the top of the page on query param change

[taion]

Scrolling to top is expected as default behavior I think - if you had a normal link to the same page with different query parameters, it's the behavior you'd get.

[taion]

@refast brings up a good suggestion at #2137 (comment) for adding some sort of onChange handler. I'm not sure how exactly we'd set it up, because I don't think e.g. we'd want to use that handler for URL parameter changes (and if we don't, then the name is a bit confusing). onQueryChange perhaps?

[agundermann]

I'm kinda surprised that onEnter was changed to fire on params change to begin with. I assumed the purpose of onEnter was to provide a hook that can't be implemented in pure React (doing something possibly async before the component is mounted). That's not the case for changing params though since we have componentWillReceiveProps.

If we do need to fire something in response to changing params/query I think having a separate onChange makes more sense than using onEnter for everything.

[taion]

@taurose You mean even for the case where you go from e.g. /widgets/3 -> /widgets/4? It seems reasonable to me to model that as leaving the former route and entering the latter.

[agundermann]

Yeah, it's certainly reasonable. I thought it used to be different, but looks like I was wrong anyway :)

[ikornaselur]

Couldn't those routes be configured something like this:

<Route path="widgets" component={ManyWidgets} onEnter={::this.enterManyWidgets}>
  <Route path=":id" component={SingleWidget} onEnter={::this.enterSingleWidget}/>
</Route>

Where going from /widgets/3 -> /widgets/4 should be leaving (and entering) the SingleWidget component route, but not the ManyWidgets one? Because at the moment it works just like that, except when changing the params.

[taion]

Yup, so assuming your routes are:

<Route path="widgets" component={WidgetParent}>
  <Route path=":id" component={WidgetChild} />
</Route>

There are two transition categories that are relevant -

• /widgets/3 -> /widgets/4
• /widgets/3?foo=bar -> /widgets/3?foo=baz

Bonus:

• /widgets/3?foo=bar -> /widgets/4?foo=baz

In each case, what hooks do we run on WidgetParent and WidgetChild?

[sikanhe]

So if you think the default behavior should be scrolling up when param/query param changes(only state change no actual page transition), then how would I go about preventing it? It seems much harder to preserve scroll than it is to add scrollTo(0,0).

See #477 and #439

[taion]

That's a separate question.

New lifecycle hooks should be post-1.0. Why don't we just roll back #2137 (but keep the test cases) for 1.0.0?

[mjackson]

Agreed. Let's roll back #2137. My bad :(

[taion]

Ah crud I didn't see that you self-assigned that. Oh well.

[refast]

Another solution is an ability to manually reload a route/state. Something like:

this.history.pushState(null, '/url/to/current/route', newQueryParams).reload();

Then If you want to reload route after query params changed, you have to do it manually.
And reloading will run onEnter hook.

[schnerd]

my bad on #2137. I had been using willTransitionTo from 0.11.6, which does fires on query param changes. The upgrade guide made it sound like onEnter was the equivalent in 1.0 so I assumed it was a bug when query changes didn't trigger onEnter.

I'm in agreement that "onEnter" should not be called when query params change, however it would be nice if react-router offered a hook for this–something like the onChange @refast mentioned. My current solution is to put logic in componentWillReceiveProps, but it's not an ideal place for calls to replaceState. Looking forward to more discussion post-1.0

[shaimo]

I'm trying to upgrade from previous versions to the current 1.0(.0-rc4) and got a bit confused as to whether this was fixed or not. It doesn't seem to work for me (onEnter not called when only the query changes). If not fixed intentionally - what's the suggested workaround? In my case the query includes a page number (among others), and the page transition is used to trigger the loading of the required data according to that page number. So I need it to be called e.g. when the user clicks to get the next page... Thanks

[taion]

You should not be using onEnter for data loading. I don't know if we have more details on recommended ways when using e.g. Flux or Redux; you can take a look at e.g. how react-router-relay handles it w/Relay integration, but it's really not ideal to completely block a transition to load data, and you shouldn't count on having support for use cases built around doing that.

[shaimo]

Sorry for not providing enough details - I'm using flux and am not using the route transition for data loading, but rather just to asynchronically trigger the loading of the required data. No blocking takes place. I guess I'll need to change how I trigger that...

[taion]

We discussed this a bit on #rackt on Reactiflux. You'll probably be happier to use e.g. createComponent to trigger data fetching. We'd be happy to discuss more there or over SO - the issue tracker isn't really a great format for this sort of "how do I do X" discussion.

[kadmil]

Guys, is there any good SO questions on the issue? I've made one, because can't see any solutions around. BTW, onChange sounds nice.

http://stackoverflow.com/questions/33671515/firing-callback-on-query-change-in-react-router

[taion]

Let's continue discussion on #2547.