Don't diff memoized host components in completion phase #13423
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gaearon
changed the title
gaearon
commented
Aug 17, 2018
| @@ -357,8 +357,8 @@ exports[`ReactDebugFiberPerf skips parents during setState 1`] = ` | |||
|
|
|||
| ⚛ (Committing Changes) | |||
| ⚛ (Committing Snapshot Effects: 0 Total) | |||
| ⚛ (Committing Host Effects: 6 Total) | |||
| ⚛ (Calling Lifecycle Methods: 6 Total) | |||
There was a problem hiding this comment.
Should have spotted this earlier. Sigh.
There was a problem hiding this comment.
Is this just way more efficient now?
There was a problem hiding this comment.
I think not — I think it was still overcounting actual updates (not entirely sure why). But it's better.
Details of bundled changes.Comparing: d14e443...0a477d8 react-noop-renderer
Generated by 🚫 dangerJS |
sebmarkbage
reviewed
Aug 17, 2018
| @@ -361,6 +361,10 @@ function completeWork( | |||
| // If we have an alternate, that means this is an update and we need to | |||
| // schedule a side-effect to do the updates. | |||
| const oldProps = current.memoizedProps; | |||
| if (oldProps === newProps) { | |||
| break; | |||
There was a problem hiding this comment.
Be careful with those early returns. I think you’re skipping the ref changing.
There was a problem hiding this comment.
Can it even change if we bail out?
There was a problem hiding this comment.
I'll push up a fix tho.
There was a problem hiding this comment.
In theory we can have a different element with the same props object.
Merged
This was referenced Sep 7, 2018
This was referenced Sep 7, 2018
This was referenced Nov 1, 2018
This was referenced Sep 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #13425.
View without whitespace: https://github.com/facebook/react/pull/13423/files?w=1
The bigger issue (#13424) is still a problem but it has always been broken (I checked as far as 0.11). Here I'm only fixing the regression in React 16 that made this a problem even when
setStatecalls are in different components.In particular, we shouldn't be diffing host components that are outside of
setStatepath.Some indentation changes are due to tests I tweaked but they're unrelated. The only change there is to consistently add and remove
containerfrom the document. The real changes are only in the newly added test at the bottom, and inCompleteWork.