Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fixed] Now execute willTransition* hooks even if only query part was changed #499

Merged

Conversation

vladimir-vg
Copy link
Contributor

Upgraded to 0.11 and discovered that params now passed a bit differently. I had rewritten my app code to use willTransitionTo instead of old componentWillReceiveProps.

I'm using query params for filtering results. Surprisingly discovered that now query changes doesn't fire hooks, while still producing transition.

This pull request fixes that.

@vladimir-vg vladimir-vg changed the title Execute willTransition* hooks even if only query part was changed [fixed] Now execute willTransition* hooks even if only query part was changed Nov 23, 2014
@ryanflorence
Copy link
Member

are you saying componentWillReceiveProps is not called anymore on query changes?

@mjackson
Copy link
Member

@vladimir-vg Can you please write a test that demonstrates the problem you're trying to address with this PR?

@vladimir-vg
Copy link
Contributor Author

@rpflorence yes, exactly. Well, componentWillReceiveProps is not called at all, because now we access params and queries using State mixin and willTransition* hooks.

And these willTransition* hooks ignored if only query part was changed.

@mjackson, I will write tests, but cannot do it right now -- probably in 2-3 days, when I reach my computer.

… changed

Previously query change didn't fire hooks. Now does.
@vladimir-vg
Copy link
Contributor Author

@mjackson added test that demonstrates problem. If you comment added changes, test will fail.

@ryanflorence
Copy link
Member

@vladimir-vg ignoring the potential merge of this pull request, you can pass the params/query down your view hierarchy to get the componentWillReceiveProps hook to fire (and therefore keep your app similar to how it used to be) by using the technique described here: https://github.com/rackt/react-router/blob/master/UPGRADE_GUIDE.md#thispropsparams-and-thispropsquery

@ryanflorence
Copy link
Member

wait a sec, look at the master-detail example, componentWillReceiveProps is firing there.

https://github.com/rackt/react-router/blob/v0.11.1/examples/master-detail/app.js#L92-L94

That's how it knows to get a different contact from the store.

@ryanflorence
Copy link
Member

oh ... its the query that doesn't trigger componentWillReceiveProps anymore I guess?

Maybe it should.

@vladimir-vg
Copy link
Contributor Author

@rpflorence Probably I unintentionally mislead you. I actually don't know does it fire componentWillReceiveProps. I don't care, because I do not pass params and query to Handler via props anymore. Actually it should fire.

According to 0.10 release params and query doesn't passed to Handler by default. So I do not use them in old style anymore. I get them through State mixin and by willTransition* hooks. And I had a problem with latter.

I'm aware of available workaround.

ryanflorence added a commit that referenced this pull request Dec 15, 2014
[fixed] Now execute willTransition* hooks even if only query part was changed
@ryanflorence ryanflorence merged commit 0c3c75d into remix-run:master Dec 15, 2014
@ryanflorence
Copy link
Member

thanks!

@vladimir-vg
Copy link
Contributor Author

You're welcome.

@mjackson
Copy link
Member

There are a few problems with 0c3c75d, namely:

  1. There is a bug that occurs when state.routes is empty. See Uncaught TypeError: Cannot read property 'length' of undefined #623
  2. This only triggers the transition hooks in the leaf route handler when the query changes, not the rest of the hierarchy

The first issue isn't a huge one. I can fix it easily.

The bigger issue is the second. If we're going to trigger transition hooks when the query changes, we should probably trigger them in every route handler in the hierarchy, not just the leaf route. This makes it even more obvious how the transition hooks are different from componentWillMount and friends: because they don't necessarily have anything to do with components mounting and unmounting. Instead, they're strictly concerned with routing changes.

If you guys can agree on these points I think I can make a patch today.

mjackson added a commit that referenced this pull request Dec 17, 2014
This commit fires transition hooks on all route handlers in the
hierarchy whenever the query changes, not just the leaf route.

This also fixes a regression in #499 that throws a TypeError when
there are not yet any routes in the router state (see #623).

Fixes #623
@vladimir-vg
Copy link
Contributor Author

@mjackson As I can see you already fixed the problem. Everything ok now, right?
Sorry, my bad.

@mjackson
Copy link
Member

@vladimir-vg Yeah, np :)

@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants