-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 RichText rerendering when it shouldn't #12161
Conversation
The prepareEditableTree and onChangeEditableValue function stacks would have a new reference on every render. This prevents that by memoized the stack based on the previous stack and the newly added function.
* | ||
* @return {Object} The annotations props object. | ||
*/ | ||
const getAnnotationObject = memize( ( annotations ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of memoizing function, I think it would be better for the registerFormatType
function to do the memoization itself using a shallow
comparison. This way it would work with any implementation of __experimentalGetPropsForEditableTreePreparation
That said, we're kind of running out of time, so I'm not considering this as blocking at the moment as it's a forward-compatible change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, this sounds better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirm that this does fix the Yoast incompatibility issue.
@@ -4,6 +4,8 @@ | |||
import createSelector from 'rememo'; | |||
import { get, flatMap } from 'lodash'; | |||
|
|||
const emptyArray = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I think in other places we called this EMPTY_ARRAY
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't be bad to have the same comment:
gutenberg/packages/editor/src/store/selectors.js
Lines 57 to 66 in 563ef41
/** | |
* Shared reference to an empty array for cases where it is important to avoid | |
* returning a new array reference on every invocation, as in a connected or | |
* other pure component which performs `shouldComponentUpdate` check on props. | |
* This should be used as a last resort, since the normalized data should be | |
* maintained by the reducer result in state. | |
* | |
* @type {Array} | |
*/ | |
const EMPTY_ARRAY = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I think in other places we called this
EMPTY_ARRAY
.
Related guideline: https://github.com/WordPress/gutenberg/blob/master/docs/contributors/coding-guidelines.md#constants
@@ -119,6 +120,14 @@ export function registerFormatType( name, settings ) { | |||
|
|||
dispatch( 'core/rich-text' ).addFormatTypes( settings ); | |||
|
|||
const emptyArray = []; | |||
const getFunctionStackMemoized = memize( ( previousStack = emptyArray, newFunction ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work to keep the cache size to one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why do both this and prepareEditableTree
need to be memoized? Could use some more clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not as we need one per RichText I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, because a different function could be returned based on richTextIdentifier
/blockClientId
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering because it's probably best to keep cache size as small as possible. I'm guessing the issue is just consecutive calls and nothing else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So how big is this cache going to grow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No way to cache per instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question though
@@ -119,6 +120,14 @@ export function registerFormatType( name, settings ) { | |||
|
|||
dispatch( 'core/rich-text' ).addFormatTypes( settings ); | |||
|
|||
const emptyArray = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Just wondering, does this also fix #11595 (comment)? |
8ba55a9
to
2fc48e9
Compare
@iseulde In my testing it doesn't fix it completely |
That is what I concluded too. It's probably not far away. I will dive into that further to completely fix it. But I thought that doesn't have to be in 4.5.1. |
/** | ||
* External dependencies | ||
*/ | ||
import memize from 'memize'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see this added as a dependency.
@@ -26,6 +26,7 @@ | |||
"@wordpress/i18n": "file:../i18n", | |||
"@wordpress/rich-text": "file:../rich-text", | |||
"lodash": "^4.17.10", | |||
"memize": "^1.0.5", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need it on the server side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we don't, there's no script for memize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, cool then :)
closes #12150
Description
The prepareEditableTree and onChangeEditableValue function stacks would have a new reference on every render. This prevents that by memoized the stack based on the previous stack and the newly added function.
How has this been tested?
Tested using Yoast SEO. I am working on e2e tests to test that RichText's doesn't rerender too often.
Screenshots
Types of changes
Checklist: