fix(react): always attempt to cleanup editor instances, starting from creation #5492 #5496
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
🦋 Changeset detectedLatest commit: 4a94b17 The changes in this PR will be included in the next version bump. This PR includes changesets to release 54 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Changes Overview
React.StrictModehas made the way that we create editor instances very strange. There is a tension where React wants for us to create side-effects only within auseEffectso the lifecycle that StrictMode goes through is very strange:This is meant to understand whether the functions are indeed memory safe, which I can appreciate but it is very different behavior from what it actually does at runtime:
So this change makes it safe to run useEditor in StrictMode, our test suite was not previously set up to test StrictMode behavior so it was missed in original development.
Implementation Approach
The core of the change ended up being quite simple, because we can create the editor within the first render, we need to already schedule it's destruction.
Scheduling a destruction, ensures that an instance that was created in that first render pass can be cleaned up.
Waiting one more tick than before ensures that we don't accidentally destroy an editor instance that could actually be valid in the next render pass.
In StrictMode, there will be two editor instances created, the first will be created & quickly destroyed in 2 ticks.
In Normal React, there will only ever be 1 instance created and destroyed only on unmount.
Testing Done
Lots of manual testing and console logging, we are not set up in a way to test React rendering behavior so tests could not be written
Verification Steps
Collaboration Cursors are a great way to test this, because if two instances are active attached to the same ydoc, their clientids will be the same and it will constantly try to send awareness updates even though they are attached to the same instance.
This manifests itself visually too by constantly flickering.
The silver lining with the original bug is that when React was running in normal mode (like in prod), the behavior was actually working properly, it was only an issue in StrictMode
Additional Notes
Checklist
Related Issues
#5492