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

fix(Transition): Stale nodes in transition callbacks #469

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Mar 12, 2019

The issues is that findDOMNode is only called a single time after componentDidUpdate and the return value is used for every transition callback that is triggered by this update. If the child however changes its DOM node while transitioning this value is stale.

Edit: This is now a breaking change from 4.4.0 because #559 changes the signatures of on* callbacks when nodeRef is used. I think this behavior is unnecessary and makes it harder to reason about the component.

@iamandrewluca
Copy link
Contributor

iamandrewluca commented May 5, 2020

@silvenon I think this can be closed. See #559

@eps1lon
Copy link
Collaborator Author

eps1lon commented May 5, 2020

@silvenon I think this can be closed. See #559

I'll rebase and see if the bug persists. But I suspect this still happens for Transition using findDOMNode.

@eps1lon eps1lon mentioned this pull request May 8, 2020
@iamandrewluca
Copy link
Contributor

iamandrewluca commented May 8, 2020

As a note. When user is passing nodeRef prop, node should not be passed to callback functions.

@iamandrewluca
Copy link
Contributor

iamandrewluca commented May 8, 2020

The issues is that findDOMNode is only called a single time after componentDidUpdate and the return value is used for every transition callback that is triggered by this update. If the child however changes its DOM node while transitioning this value is stale.

I'm looking into code and it seems after #559 statement above is false one?!
Or I don't understand exactly what you mean.

Also current implementation in this PR, it seems just to add back node in callback functions. this.getNode does not add too much value 🤔

@eps1lon
Copy link
Collaborator Author

eps1lon commented May 9, 2020

this.getNode does not add too much value thinking

I could inline it sure. But it's used often enough that I think abstracting it is warranted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants