Skip to content
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

Use memoization to avoid unnecessary textContent updates in TextString component #5306

Merged
merged 1 commit into from
Feb 24, 2023

Conversation

clauderic
Copy link
Collaborator

@clauderic clauderic commented Feb 22, 2023

Description
This PR changes the approach used in the <TextString> component to synchronize the DOM and the Virtual DOM.

Previously, the initial render of the <TextString> component would render the initial text, but the second render would remove the text from the DOM, before quickly being re-committed to the DOM in the layout effect before the browser had time to paint.

This PR takes a different approach that avoids ever having to render an empty textContent on the second render. Instead, we render a memoized component in <TextString> (<MemoizedText>), that receives props that are referentially stable across re-renders, such that React never updates the DOM across re-renders.

This PR provides a significant performance improvement over the current implementation, as previously the first update to an element would cause all its text leaves to re-render and wastefully update their textContent even if it had not changed.

Issue
I couldn't find any related tickets.

Example

Before

Screen.Recording.2023-02-22.at.5.41.31.PM.mov

After

Screen.Recording.2023-02-22.at.5.40.35.PM.mov

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

@changeset-bot
Copy link

changeset-bot bot commented Feb 22, 2023

🦋 Changeset detected

Latest commit: b6dd630

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate-react Patch

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

@clauderic clauderic force-pushed the text-string-memoization branch 3 times, most recently from 63e9f31 to 9052328 Compare February 22, 2023 22:47
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@clauderic clauderic marked this pull request as ready for review February 22, 2023 22:59
@clauderic clauderic mentioned this pull request Feb 23, 2023
5 tasks
Copy link
Collaborator

@dylans dylans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks promising. Going to try this out in our app in collaborative editing mode and see if it introduces any regressions.

@dylans dylans merged commit 213edbb into main Feb 24, 2023
@github-actions github-actions bot mentioned this pull request Feb 24, 2023
@clauderic clauderic mentioned this pull request Feb 24, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants