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

perf: optimizations for composer with Tiptap #6315

Merged
merged 4 commits into from
Nov 23, 2024

Conversation

nperez0111
Copy link
Contributor

@nperez0111 nperez0111 commented Nov 13, 2024

Hi Blue Sky team,

I'm one of the maintainers of Tiptap 👋

I heard that you guys were using Tiptap in your composer, which is fantastic! I love what you are building, so I thought I could add some optimizations of my own.

I recently rewrote our React binding to be much more performant, but due to our large user-base I had to make the perf gains opt-in to not break our thousands of users.

That's what the shouldRerenderOnTransaction: false is, by default in Tiptap version 2 we re-render every time that a transaction happens in the editor, a transaction is fired on:

  • any text selection update
  • focus & blur
  • any changes to content
  • pretty much any event you can think of happening within the editor

This is an important change to make, because the editor can be very sensitive to re-renders, in the worst case this could cause missed characters if the view can't keep up.

I added a console.count like so:

diff --git a/src/view/com/composer/text-input/TextInput.web.tsx b/src/view/com/composer/text-input/TextInput.web.tsx
index 5f3ff850c..6ffc82f74 100644
--- a/src/view/com/composer/text-input/TextInput.web.tsx
+++ b/src/view/com/composer/text-input/TextInput.web.tsx
@@ -250,6 +250,7 @@ export const TextInput = React.forwardRef(function TextInputImpl(
     },
     [modeClass],
   )
+  console.count('render')
 
   const onEmojiInserted = React.useCallback(
     (emoji: Emoji) => {

And took a screen recording of before:

Screen.Recording.2024-11-13.at.22.32.47.mov

And, after the change was made:

Screen.Recording.2024-11-13.at.22.33.21.mov

You can really see the difference in the number of renders!

In an ideal world, the editor would be isolated to it's own component that would render only once. This is quite achievable even in very large apps. But, because of how things are currently wired up (using a controlled component pattern), there is only so much perf we can get out of this without a larger refactor.

Given that users are not going to be editing very large documents, I'd say that it is not so important to get this perfect, but for future reference, a more performant pattern would be to read the editor's current state when you need it. Either through the useEditorState hook, or by reading from the editor instance only when you need the value like on submission.

I've also went ahead and updated the tiptap packages to more recent versions, we strictly follow semver and have thousands of people running these packages every day.

If you've made it this far, I very much appreciate your time and wish you a bright blue sky of a day!

@gaearon
Copy link
Collaborator

gaearon commented Nov 13, 2024

Thank you so much! Looks like the recordings didn't go through. Regardless, will have a look at this soon.

@gaearon
Copy link
Collaborator

gaearon commented Nov 13, 2024

Re: layout effect, I think there was a reason for it but can't remember.. Maybe it's applying it to a different node?

@nperez0111
Copy link
Contributor Author

Appreciate it @gaearon, I use the GH CLI to make recordings so I have to upload them after

}, [t, fonts])

React.useLayoutEffect(() => {
let node = editor?.view.dom
Copy link
Contributor Author

@nperez0111 nperez0111 Nov 13, 2024

Choose a reason for hiding this comment

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

editor.view.dom is actually referencing the same DOM node as the one that EditorContent renders. Which is why this change was made

You can see EditorContent here is just trying to get a ref to a react mounted empty div (and we initialize the editor from there in pure JS)

Copy link
Collaborator

Choose a reason for hiding this comment

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

interesting, i'll need to have a look! i remember trying to do it there but it didn't work for some reason, maybe i made a mistake though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I did double-check on my end, but always good to have a second look!

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

thank you so much for the PR! i could not find any problems, going to give it a go

@gaearon gaearon merged commit fdc3f0f into bluesky-social:main Nov 23, 2024
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