Themes: Avoid DOM reconciliation for existing themes on scroll #2915
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
The theme component was using the pure render mixin to avoid reconciliation on re-render. Due to the complex nature of the props, the reconciliation was never being short-circuited. Implement a custom shouldComponentUpdate method that does a deep comparison on the theme object prop.
seear
added
[Feature Group] Appearance & Themes
| @@ -62,6 +61,10 @@ var Theme = React.createClass( { | |||
| actionLabel: React.PropTypes.string | |||
| }, | |||
|
|
|||
| shouldComponentUpdate: function( nextProps ) { | |||
| return ! isEqual( nextProps.theme, this.props.theme ); | |||
There was a problem hiding this comment.
Shouldn't comparing theme.id and theme.active be sufficient here?
There was a problem hiding this comment.
Shouldn't comparing theme.id and theme.active be sufficient here?
Yeah you are right there, but the fact that there may not be a theme obj supplied (in the placeholder usage case) pushed me to go for isEqual as the simpler, more readable approach. Make sense, or should we just compare the relevant fields since this is on the speed-critical path?
There was a problem hiding this comment.
To prevent headaches later, it's better to compare the whole theme object — it shouldn't be horrendously slow.
|
Code looks good. |
seear
added a commit
that referenced
this pull request
Feb 1, 2016
Themes: Avoid DOM reconciliation for existing themes on scroll
nylen
removed
the
[Status] Needs Review
This was referenced Jan 12, 2021
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.
Before
After
These tables show time wasted reconciling component tree sections that have not changed. The start/stops have been placed around the
fetchNextPage()call in the<themes-list-fetcher/>, like this:This prints a table every time a scroll causes the next page of themes to be fetched and rendered. The tables above are towards the end of the entire set of themes.
The theme component was using the pure render mixin to avoid
reconciliation on re-render. Due to the complex nature of the
props, the reconciliation was never being short-circuited.
This PR Implements a custom shouldComponentUpdate method that does a deep
comparison on the theme object prop. We can see from the second table that no time is now wasted attempting to reconcile the huge amount of existing themes on the page.
To Test