Skip to content

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

Open
eps1lon wants to merge 2 commits into
reactjs:masterfrom
eps1lon:fix/stale-nodes
Open

fix(Transition): Stale nodes in transition callbacks#469
eps1lon wants to merge 2 commits into
reactjs:masterfrom
eps1lon:fix/stale-nodes

Conversation

@eps1lon

@eps1lon eps1lon commented Mar 12, 2019

Copy link
Copy Markdown
Member

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.

@unrevised6419

unrevised6419 commented May 5, 2020

Copy link
Copy Markdown

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

@eps1lon

eps1lon commented May 5, 2020

Copy link
Copy Markdown
Member Author

@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
@eps1lon eps1lon force-pushed the fix/stale-nodes branch from 3b93c8d to 7093cd6 Compare May 8, 2020 10:25
@unrevised6419

unrevised6419 commented May 8, 2020

Copy link
Copy Markdown

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

@unrevised6419

unrevised6419 commented May 8, 2020

Copy link
Copy Markdown

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

eps1lon commented May 9, 2020

Copy link
Copy Markdown
Member Author

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.

2 participants