[Bug]: Bad performence of ReactRenderer

[fantasticit]

Which packages did you experience the bug in?

react

What Tiptap version are you using?

2.0.1

What’s the bug you are facing?

The render method of ReactRenderer is called every time I type, which causes the editor to lag. Is there a way to improve this? Also, is it possible to add a viewport judgment inside the NodeViewContent to determine whether to render it or not to optimize performance?

import React from 'react';

import { Node } from '@tiptap/core';
import { NodeViewContent, NodeViewProps, NodeViewWrapper, ReactNodeViewRenderer } from '@tiptap/react';

export const CardView: React.FC<NodeViewProps> = ({ editor, node, getPos }) => {
  return (
    <NodeViewWrapper>
      {/* {some other element} */}
      <NodeViewContent />
    </NodeViewWrapper>
  );
};

export const Card = Node.create({
  name: 'card',
  group: 'card',
  content: 'block+',
  inline: false,
  isolating: true,
  marks: '',

  addNodeView() {
    return ReactNodeViewRenderer(CardView);
  },
});

Document.extend({ content: `card+` })

What browser are you using?

Chrome

Code example

No response

What did you expect to happen?

improve performence

Anything to add? (optional)

No response

Did you update your dependencies?

• Yes, I’ve updated my dependencies to use the latest version of all packages.

Are you sponsoring us?

• Yes, I’m a sponsor. 💖

[C-Hess]

The render method of ReactRenderer is called every time I type, which causes the editor to lag

I don't think there's any safe optimizations to be made for this behavior that can be handled by the library itself, or without a breaking change. The editor object, which is in charge of maintaining TipTap's internal state, is passed down to all React node views. As a result, every change to the editor should result in a re-render of node views. Otherwise, node views that depend on this behavior and have side effects that require a re-render will no longer be able to do so.

I wonder if a useEditor hook could be useful here. Node views may not need to re-render as frequently since only node views that use this useEditor hook would re-render when the editor content is changed outside of the node's "domain."

Anyways, if you are worried about performance when rendering a complicated node view, you can instead create an inner component that uses React's memo higher-order-function. Then, make it so your component only accepts props that, if changed, should trigger a re-render. You should get virtually the same performance benefits you're likely seeking.

is it possible to add a viewport judgment inside the NodeViewContent to determine whether to render it or not to optimize performance

By "viewport judgment", are you potentially referring to virtual scrolling techniques that unmounts components that are not visible on the screen? There are many libraries dedicated to this problem, react-virtualized being one of them. Unfortunately, implementing something like this may also have issues, as the core of TipTap is just vanilla JS, which would likely make integrating with a React-centric solution a challenging problem. It might not be worth the effort to look into this, given that rendering too many elements on the screen usually isn't the largest bottleneck as opposed to the complexity of React components. Otherwise, the react-virtualized may be a good place to start.

[totorofly]

I've used both Vue and React versions of NodeView, and I've noticed that under the exact same text content and configuration conditions, the React version tends to be more laggy when quickly adding or deleting lines. This does indeed affect the user experience negatively.

There should still be considerable room for optimization and improvement in ReactNodeViewRenderer. After switching to pure JavaScript code, the lag during rapid operations noticeably decreased. This might also partially suggest that the current performance issues are not inherently due to React itself, but rather are likely due to how ReactNodeViewRenderer is implemented or configured.

[AdventureBeard]

The ReactNodeView Renderer is so convenient, but indeed has a huge performance overhead. @dwzkit , you have any tips for migrating to plain NodeViews? I hate to lose access to my React component library, but the user experience of my application suffers greatly when using ReactNodeViews, adding 200-1000ms of delay to every interaction.

[totorofly]

The ReactNodeView Renderer is so convenient, but indeed has a huge performance overhead. @dwzkit , you have any tips for migrating to plain NodeViews? I hate to lose access to my React component library, but the user experience of my application suffers greatly when using ReactNodeViews, adding 200-1000ms of delay to every interaction.

I’ve found a method to improve performance by handling the mouseEnter event listener in nodeView using vanilla JavaScript. I then use the Events BUS(Singleton pattern) to pass data such as the node, getPos, and Editor from nodeView to React components for further processing. For tasks in the React component that require ProseMirror processing (like setting the background color of a selected block), I utilize Decorations. This approach bypasses the low-performance nodeView options in tiptap, resulting in a significant performance boost.

[WilliamIPark]

This is also an issue our team has fallen into. Having every NodeView update with new props per change is rough for performance, although I do understand why it's currently needed.

I've been taking a look at what react-prosemirror are doing for a better React implementation, and just wanted to share in case it helps come up with a better implantation for Tiptap.

[WilliamIPark]

Something I would like to see to help us work around this issue would be a getter function for the the editor that is referentially stable, in addition to the editor being passed in directly.

Most of the time I'm directly interacting with the editor prop is when I need to fire off a command, or read from it as part of a callback triggered by some event.

Consider this simple example:

function MyNodeView({ editor }) {
  const handleDoAThing = useCallback(() => {
    // Maybe I'm reading from the editor
    const json = editor.doc.content.toJSON();
    // Maybe i'm firing off a command
    editor.commands.doAThing();
  }, [editor]);

  return (
    <div>
      <SomeComponent doAThing={handleDoAThing} />
      <NodeViewContent />
    </div>
  );
}

In this case, because the editor is always changing, <SomeComponent /> is also going to need to often re-render, because the dependancy in the useCallback updates.

If we could instead do something more like this, we would be able to stop all children from re-rendering, because getEditor is stable, so our callback won't update.

function MyNodeView({ getEditor }) {
  const handleDoAThing = useCallback(() => {
    const editor = getEditor();
    const json = editor.doc.content.toJSON();
    editor.commands.doAThing();
  }, [getEditor]);

  return (
    <div>
      <SomeComponent doAThing={handleDoAThing} />
      <NodeViewContent />
    </div>
  );
}

We will still have the issue of the root NodeView component updating (which on its own shouldn't be too expensive), but it means for expensive UI that still need to interact with the editor, we can give them a bit of shelter from the editor changing by making them children.

[bdbch]

Nice idea! Not sure if I can find some time for this soon but it's a nice idea we should keep on track.

[C-Hess]

@WilliamIPark @bdbch, the above solution has some issues, unfortunately. The biggest issue is that it breaks React design patterns for "reactivity". The editor object fundamentally should not be referentially stable. Why? Because it breaks use cases like this:

const MyRenderView = ({ getEditor ) => {
    const editor = getEditor();
    if (editor.state.selection()) {
       // TECHNICALLY, this will still work, but only because MyRenderView is rendered on every editor update
       // However, as we've been discussing, this is a performance issue as well. If we wanted to make render views
       // not re-render unless the underlying node is updated, editor.state will become stale :(
    }
}

Some alternatives to consider are splitting out the editor into two "objects": the editor "state", and the editor "dispatch" (ie commands). The latter can designed to be referentially stable. We could make it so that by default, react node views only update if the node they are associated with are changed, but then a hook could be created to get the editor from within a render view (I think react-slate does something similar if I recall correctly). to get the editor state:

const MyRenderView = ({ commands, node }) => {
    // By default, MyRenderView could be designed to only render by TipTap if the underlying Node changes
    // but if you wanted your node view to react to editor changes, you could pull the editor reference from a
    // context using a special hook
    const editorState = useEditorState();

    const myStableCallback = useCallback(() => {
       
    // Using commands will be okay (and operates similarly to React's useReducer's `dispatch`) because
    // commands can be referentially stable
    }, commands]);
}

I might spend some time this next week seeing what the above approach would look like and if there are other pitfalls. It would change a lot of patterns on the React-side of things, but it falls in line more with how React is supposed to operate with things like this IMO.

In the meantime, you can simply omit the editor from callback dependency arrays. Not ideal, but it should achieve the same result.

[WilliamIPark]

Glad to get some discussion going on about this, thanks @C-Hess.

The use-case you mention would be a problem, which is why I'd opt for the getEditor to be an addition to the editor prop, then editor could be used in the cases that you mention. But you're 100% right in saying that it could be a pitfall, especially for some users who don't understand why both are available. An advantage is that it wouldn't be a breaking change, which is why I thought it worth mentioning.

That said, I'm a fan of your approach. Even if it meant having to fix some breaking changes I'd rather the ReactRenderer API used something like that for sure, saying this seems like a fundamental flaw in this API. Keep me updated if you do more experimentation with it!

@bdbch Thanks for the reply, I appreciate you guys are busy. If an acceptable community PR was put together to solve or aid this, do you reckon the team could find a way to review and merge it sooner rather than later?