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

Transition From hooks regression #101

Closed
ryanflorence opened this issue Jul 22, 2014 · 5 comments
Closed

Transition From hooks regression #101

ryanflorence opened this issue Jul 22, 2014 · 5 comments

Comments

@ryanflorence
Copy link
Member

a64d0cd doesn't send the right refs:

a64d0cd#diff-44cf2443bda506f43988136136914298R378

These are the refs for the next route, not the current, and so this code doesn't send the right component (and sometimes no component at all)

a64d0cd#diff-44cf2443bda506f43988136136914298R386

Open up the examples/transitions to see the problem.

I'm not sure how to access the ReactCompositeComponent in order to get the correct refs.

@mjackson
Copy link
Member

Confirmed that I see the problem as well. I'm looking into it..

@mjackson
Copy link
Member

This regression was caused by 73570ed.

I'm an idiot because I approved the merge but I already knew that we would need to change some things to make willTransitionFrom hooks work properly, I just forgot when I merged.

The problem is that when we switched from this.props.activeRoute => this.props.activeRoute(props, children) our top-level route is no longer the lowest thing on the render stack when refs are created.

Two possible solutions:

  • Roll back this.props.activeRoute(props, children). There was never a solid use case for it afaict. Just something that sounded like we should do it.
  • Figure out a way to drill down the render stack so that refs still work.

@mjackson
Copy link
Member

I'm going to fix the bug by rolling things back to how they were before the merge. We can reopen #59 and discuss there.

@mjackson
Copy link
Member

Just noticed your updated data-flow example uses the props. This is the first convincing use case I've seen for route props.

@ryanflorence
Copy link
Member Author

Yeah, its the only case that makes sense, but I'm still happy to roll it back if getting the correct refs proves too difficult / strange. The inability to couple the route handlers forces a more decoupled, flux-style approach to your app.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 25, 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

No branches or pull requests

2 participants