-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
[Bug]: Usage of ReactNodeViewRender drastically reduces performance for large Text #4492
Comments
I have the same issue. Terrible terrible performance with just a few react node views. |
I noticed the same. In my case, it seems to be related to when the
The problem is that A better solution, assuming it is easy to track the currently selected node, would be to only deselect relevant nodes, i.e. something like:
From what I've tested, interestingly enough in my case, I could just remove the |
To follow up a bit on my own reply: I did notice a change in behavior in some cases, as one would expect with the naive solution I suggested. A more proper solution that, from what I can tell, keeps the behavior identical but also improves the performance, is essentially any solution that prevents the unnecessary One temporary workaround could be to e.g. add a separate
...and change
Now, it seems that only the relevant nodes re-render, which in my cases of multiple thousands of nodes is very noticeable. A lot of unnecessary calls to |
edit: There is a flaw with this patch that is addressed in a different post below. I experienced a similar problem: text input in documents with many react node views was so slow as to make it unusable. There are probably a few performance optimizations to be had, but I found my issues to be related to #3580, which called out this method: tiptap/packages/react/src/EditorContent.tsx Lines 72 to 82 in 42039c0
// EDIT: see updated patch below
// import { PureEditorContent } from "@tiptap/react";
//
// PureEditorContent.prototype.maybeFlushSync = (fn: () => void) => {
// fn();
// };
|
this fixed for me aswell |
I confirm this is an issue for my team too! Removing the With unpatched TipTap I see my custom nodes re-render on every keystroke and it gets very problematic when the document has tens of them. With the patch, I see only the custom node I am editing re-render. EDIT: Digging around with the React Profilers, I see many more React render commits (hundreds vs a handful) with flushSync enabled. EDIT 2: digging through the blames in ReactNodeViewRenderer it seems as if the flushSync behavior was introduced to fix this bug and judging from the commit message it was intended to render node views immediately when created, but what seems to be happening here is that they are rendered on every keystroke, which was not the intended behavior. |
Thanks for digging into that. I think you're right that flushSync is only meant to run once, after the component mounts. I've modified my patch and I believe it fixes some odd interactivity issues I had been experiencing, while also showing the same performance improvements. For some reason I didn't think to attribute my issues to this change. PureEditorContent.prototype.maybeFlushSync = function maybeFlushSync(fn) {
if (this.initialized) {
flushSync(fn);
this.initialized = false;
} else {
fn();
}
}; |
We're experiencing similar issues as well, even after the patch above. I tried wrapping the NodeViewWrapper with a React Profiler https://react.dev/reference/react/Profiler and it looks like all the nodes are updating with every keystroke. Is this expected? |
@andrewlu0 I can only speculate without seeing your code, but I would try to rule out something in your application. The flushSync bug is a problem in the framework, but it's fairly common to cause excessive renders in the course of regular development. I'd recommend profiling with the react chrome extension and judiciously applying debounced renders and memoization. |
@fdintino I've tried a few attempts at memoization and debounced renders, but still experience significant lag when the number of React nodes reaches a few hundred, even with a very basic React node that is just a wrapper around a header. Is your application able to handle this well? Even trying the sample on their website here https://tiptap.dev/guide/node-views/react, if you copy/paste so there's around 300 copies of that yellow button node, the editor becomes very slow. |
I do experience lag and performance issues, and with hundreds of nodes it becomes unusable. But those performance issues are fixed (for me) with the overridden definition of |
Just adding another data point to the discussion. We have long documents (50+ A4 pages), with only ~ 100 React nodes in them. Performance is not yet at a standstill, but it's no longer "buttery" as we would like. Adding the monkeypatch fix above fixes it for me. Thanks @fdintino |
I am deploying to firebase and I cannot monkey patch because firebase rebuilds node_modules directory. Any way around this? |
Please can someone fix this at TipTap, because this makes TipTap with react components unusable... This should be fixed. |
Just to add another data point, the monkey-patch provided above does not improve performance for me, and changes the behavior of cursor placements after inserting nodes with React Node Views. |
@AdventureBeard are you seeing the cursor behavior changes with the first or second patch that I posted? |
We had to revert the patch as well because of some weird cursor placement bugs that were introduced after applying it. |
I wonder if there are other places where state dependencies aren't fully accounted for, but they hadn't caused issues before because this bug was rerendering all components on every keystroke. |
I am assuming you're both referring to my patch and not @Zloka's, but it's probably worth clarifying that. |
@fdintino, this one: PureEditorContent.prototype.maybeFlushSync = function maybeFlushSync(fn) {
if (this.initialized) {
flushSync(fn);
this.initialized = false;
} else {
fn();
}
}; Specifically, whenever I inserted a node with a NodeView with a content hole, the cursor would be placed after the entire node, instead of within it. WIthout the monkey-patch, the cursor is placed inside the node view after insert as expected. You can modify the insert code to account for this, but it would require code changes to all my nodes and I started to feel like I was working against Tiptap, so I put it on pause to look for other perf improvements I could make to the React node renderer. |
I'm not seeing that behavior. I was thinking of putting together a PR with this change, so if you could provide a minimal test case I would appreciate it. I would want to include the fix for that alongside this change. |
Can anyone tell me how exactly they're testing versus non-React rendering? So far I can't find any significant differences when rendering a codeblock in React versus pure JS. |
Sure. Here's a codesandbox demonstrating the problem, with a toggle between tiptap-react, a vanilla js nodeview, and a version with my patch and @Zloka's above. The number in the left margin is a count of the number of renders. |
Thanks @fdintino! That definitely makes React a lot more responsive in that example. Unfortunately it doesn't seem to bring Android speed up to vanilla JS speed as it seems to on PC, but that's understandable. |
That can be achieved by wrapping the component in const ParagraphComponentPatched = React.memo(
() => (
<NodeViewWrapperPatched className="wrapper">
<RenderCount />
<NodeViewContentPatched as="p" />
</NodeViewWrapperPatched>
),
(prevProps, nextProps) => true
); |
We have the performance issues in our pipeline and we'll probably start working on all performance related issues with React after the holidays - just as a heads up. |
The fix incorporated one of the fixes, but it didn't include my flushSync fix. I'll open that in a pull request. |
@fdintino was your flushSync fix added yet? |
@bdbch is there any update on plans for this? Or has anything already been implemented? I think it would be good to get this issue re-opened since it certainly does remain a problem (in We use a React renderer for only one node type, but it slows the editor down 10 fold in circumstances. |
Could we please get this re-opened since the issue has been noted to not have really been addressed?
|
Will there ever be follow-up on this? It's been nearly half a year. The issue persists and it should be opened, at the very least. |
Bump. The last time we heard from maintainers on this was five months ago and it's not even been re-opened, much less a single word of communication. |
Same here on version 2.4.0 |
Sorry for the silence. We will definitely take care of the issue. |
I have some ideas on how to improve this that I'll be implementing as a follow up to: #5161 Hopefully will get to it this week or beginning of next week |
following the thread here as I am in urgent need of this fix, thanks for looking into it @svenadlung! |
Running into performance issues as well |
We've prepared a hacky patch ourselves and run it in production for several months now. It's goal is to avoid portal rerenders as much as possible in ReactNodeViews https://github.com/bartlomiej-korpus/tiptap/pull/1/files @ianrtracey @bibschan let me know if you'd like to try this, I can help you prepare a yarn patch the problem with current solution is that all the ReactRenderer portals are rendered flat as part of same component(Portals), so each time any of them changes all of them rerender, and even memoization doesn't help if there's a lot of them. This happens inside In our version portals are rendered in a nested structure and at each level there is a |
This is fantastic @bartlomiej-korpus, I think your approach of avoiding re-rendering portals, and my approach of avoiding flushSyncs (instead using |
Nice @nperez0111 ! I definitely wanted to avoid |
Closeable now with the advent of |
There definitely still are some optimizations that I want to make for react node views specifically so happy to keep it open |
So I looked into the changes made with: #4492 (comment) and it suffers from the same problem that I've been seeing (where on first render focus cannot be moved into the element). This issue happens because React renders in two passes and not synchronously. My patch here eeks out a bit more performance (by not using So I think for now we can probably just apply this, see if there are drawbacks I haven't accounted for and go from there. |
Just saw the news for 2.5. Does it effectively close this issue? |
and the PR linked above is not yet merged. |
This is now resolved since 2.6 |
Which packages did you experience the bug in?
core
What Tiptap version are you using?
2.1.6
What’s the bug you are facing?
If I use this simple React Component as Wrapper for a Custom Paragraph the performance drastically reduces for large text. Luckily in my use case I don't need to use React Render for the Paragraph but there seems to be some performance bug which could be critical for other users.
I now switched to vanilla html and js for this and it works
What browser are you using?
Chrome
Code example
No response
What did you expect to happen?
Using a simple React render should not have this big of influence on the performance.
Anything to add? (optional)
No response
Did you update your dependencies?
Are you sponsoring us?
The text was updated successfully, but these errors were encountered: