Fix Editable tagName changes after #635 #661
Conversation
|
Going to move destroying there as well. |
mtias
added
[Feature] Block API
| @@ -1,5 +1,5 @@ | |||
| export default class TinyMCE extends wp.element.Component { | |||
| componentDidMount() { | |||
| initialize() { | |||
There was a problem hiding this comment.
Generally, we might want to consider conventions for component method ordering. I usually like to see:
- Static properties
- Constructor
- Lifecycle methods
- Miscellaneous
- Render
(Considering this as "miscellaneous")
| // We must prevent rerenders because TinyMCE will modify the DOM, thus | ||
| // breaking React's ability to reconcile changes. | ||
| // | ||
| // See: https://github.com/facebook/react/issues/6802 | ||
| return false; | ||
| return nextProps.tagName !== this.props.tagName; |
There was a problem hiding this comment.
I'm surprised to find that this doesn't really break. I think it'd be worth updating the preceding comment to describe when and why we allow a rerender, and ideally how it's exempt from the concern noted in the referenced issue.
There was a problem hiding this comment.
Does this sound okay?
// If the tag changes, React can reconcile the node in place after the
// editor is destroyed.| } | ||
|
|
||
| if ( nextProps.tagName !== this.props.tagName ) { | ||
| this.editor.destroy(); |
There was a problem hiding this comment.
Is this how React is able to reconcile? That we're unmounting the node altogether and React recreates it in the next pass?
There was a problem hiding this comment.
The node and contents will still be there. Is that was you mean?
There was a problem hiding this comment.
The node and contents will still be there. Is that was you mean?
It's what I was curious about, yes, but opposite of the assumption I was making. In #635 I'd encountered issues where React would reconcile based on the DOM that TinyMCE had since modified, resulting in an infinite loop of errors, particularly around splitting and merging blocks. With these changes I'm not seeing this issue, but I don't feel confident in knowing why it's not a problem.
There was a problem hiding this comment.
Hm, I'm not sure why it was a problem.
| } | ||
|
|
||
| componentWillUnmount() { | ||
| if ( ! this.editor ) { |
There was a problem hiding this comment.
Dunno if it's worth the abstraction, but I could see value in a destroy method which handles (a) verifying editor instance property, (b) destroying editor instance, (c) unsetting instance property:
destroy() {
if ( this.editor ) {
this.editor.destroy();
delete this.editor;
}
}To the point of delete vs = null;, I don't feel strongly one way or the other, but I think in cases where we assign any instance property, it would be a good idea to initialize it to its default value in constructor so that it's clearer which instance properties exist in a class.
There was a problem hiding this comment.
Yeah you're right, this is cleaner.
ellatrix
force-pushed
the
fix/heading-levels
branch
from
c80566e to
69e5bb7
Compare
|
In d7165a5 I pushed another approach which abuses - key="editor" />
+ key={ [ 'editor', tagName ].join() } />This has the same effect; namely that when |
|
I'm fine with either approach :) |
I was led to this because I'm still skeptical that React is really able to reconcile after destroying the TinyMCE instance, and that we just haven't hit the conditions where it fails hard. |
|
Yeah, we don't take any advantage of the reconciliation otherwise, so it sounds good to not do it at all. |
|
Looks good to me! 👍 |
I seems like we do need updating after a tag change.