RichText: comment isEqual checks to avoid regressing in the future #5252
Conversation
youknowriad
added
the
[Feature] Rich Text
blocks/rich-text/index.js
Outdated
| @@ -696,6 +696,10 @@ export class RichText extends Component { | |||
| this.props.tagName === prevProps.tagName && | |||
| this.props.value !== prevProps.value && | |||
| this.props.value !== this.savedContent && | |||
|
|
|||
| // Comparing using isEqual is neccessary especially to avoid unnecessary updateContent calls | |||
There was a problem hiding this comment.
Typo: neccessary => necessary. :)
youknowriad
force-pushed
the
update/comment-isequal-checks
branch
from
284b353 to
2240b4a
Compare
| @@ -696,6 +696,10 @@ export class RichText extends Component { | |||
| this.props.tagName === prevProps.tagName && | |||
| this.props.value !== prevProps.value && | |||
| this.props.value !== this.savedContent && | |||
There was a problem hiding this comment.
Just wondering: are these values ever strict equal? this.savedContent seems to change all the time, so it will never be strictly equal to the value prop? I'm also having a hard time understanding why we need to check three different values here.
There was a problem hiding this comment.
I think the idea is that before calling this.props.onChange we save the value in this.savedContent that way when the component is rerendered with the newly provided value prop. this check will fail this.props.value !== this.savedContent and we avoid calling updateContent (happens when you type continuously on a rich text element)
|
Thank you! :) |
|
Sorry for going on about this... but I want to understand the problem a bit better. Is this only a problem for multi line rich text areas? If so, why? Wy doesn't the stick equality work here? Are there other solutions to this? If not, we check for multi line instances here before we deep compare? |
|
This especially worries me since we're comparing React objects here, which, even if the content is the same might not be deep equal... |
|
@iseulde We do the comparison only if the previous checks pass, which doesn't happen so often. I don't know exactly why it's happening for multi-richtext blocks when moving the focus, I assumed it's happening because when you focusOut a new instance is generated somehow but can't tell exacly why, we'll have to look deeper. |
|
I know that it won't fire often, but it's just the principle and that it might change later on... I'd rather have we don't have to do this at all. I'll look into it a bit |
|
Fix coming :) |
Related #5100 (comment)
These checks have been added and removed several times, this comment ensures we don't remove them anymore unless we check the correct regression they introduce.