Classic block: Fix content syncing effect for React StrictMode #62051
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
Mamaduka
added
[Type] Bug
Mamaduka
requested review from
ellatrix and
tyxla
and removed request for
ajitbohra
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I also noticed that the Maybe we should defer rendering while the state is resolving? gutenberg/packages/block-library/src/freeform/edit.js Lines 45 to 69 in 513f9ea |
|
Size Change: +7 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
tyxla
reviewed
May 28, 2024
|
|
||
| const currentContent = editor.getContent(); | ||
| if ( currentContent !== content ) { | ||
| editor.setContent( content || '' ); |
There was a problem hiding this comment.
Definitely didn't make sense to do this if editor was not to be found in the first place 👍
| @@ -227,6 +230,7 @@ function ClassicEdit( { | |||
| onReadyStateChange | |||
| ); | |||
| wp.oldEditor.remove( `editor-${ clientId }` ); | |||
| didMount.current = false; | |||
There was a problem hiding this comment.
We don't do this in a lot of places. Is this necessary in practice?
There was a problem hiding this comment.
It depends. A cleanup could be necessary when you have two connected effects like this and want to ensure that one only does its job after the initial mount.
Here's an example: https://codesandbox.io/p/sandbox/heuristic-rhodes-stw52g?file=%2Fsrc%2FApp.js
|
|
||
| const currentContent = editor.getContent(); | ||
| if ( currentContent !== content ) { | ||
| editor.setContent( content || '' ); |
There was a problem hiding this comment.
Any possible concerns that this won't run once window.tinymce.get( editor-${ clientId } ) is populated?
I'm not sure. Though that change isn't really required for the StrictMode fix, I'm happy to remove it from this PR. |
ellatrix
approved these changes
May 29, 2024
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jun 4, 2024
…ress#62051) Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
patil-vipul
pushed a commit
to patil-vipul/gutenberg
that referenced
this pull request
Jun 17, 2024
…ress#62051) Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
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.
What?
PR fixes a React StrictMode bug for the Classic block. The content syncing effect triggered an error in development mode.
The bug was discovered via #61943.
Why?
The TinyMCE initialization effect was resetting the
didMountref during the cleanup.How?
didMountref when TinyMCE editor is removed.Testing Instructions
wp-env.npm run test:e2e -- editor/blocks/classic.spec.jsPatch for enabling modes
Testing Instructions for Keyboard
Same.