[Bug]: Editor options set on every render

[yurtsiv]

Affected Packages

react

Version(s)

2.11.2

Bug Description

editor.setOptions is being called on each render in case of empty deps.

The bug I ran into is quite obscure but there could be more problems.

Scenario:

const ExampleExtension = Extension.create({
   addProseMirrorPlugins() {
      return [
          new Plugin({
             view: () => ({
                update() {
                     new ReactRenderer(Component, ...) // calls flushSync inside the constructor
                }
             })
          })
      ]
   }
})

function Editor() {
  const [, setState] = useState(1)

  const editor = useEditor({...}, [])

  function onAction() {
     setState(2)
     editor.commands.setTextSelection(0) // triggers plugin view update
  }


  return (...)
}

Calling onAction will result in flushSync was called from inside a lifecycle method. error.

Event chain:

• plugin view update is triggered
• ReactRenderer calls flushSync
• Editor component gets re-rendered because of the new state
• useEditor calls editor.setOptions which again triggers the plugin view update

I believe editor options should be updated only when they change.

Browser Used

Chrome

Code Example URL

No response

Expected Behavior

No error is reported.

Additional Context (Optional)

No response

Dependency Updates

• Yes, I've updated all my dependencies.

[yurtsiv]

A simple workaround for now is to avoid that code path by adding dummy deps

const editor = useEditor({...}, [1])

[Nantris]

At a cursory glance, this looks like good low-hanging fruit.

The workaround leads to this.refreshEditorInstance running, but I'm not sure even that needs to occur?

[yurtsiv]

@Nantris Editor instance does not refresh if deps are unchanged

[Nantris]

@yurtsiv I think you misunderstand me.

I don't even think this much cheaper function needs to be called. If that's right, then we could do better still than your workaround with a proper fix.

tiptap/packages/react/src/useEditor.ts

Line 212 in 839eb47

this.refreshEditorInstance(deps)

[nperez0111]

So, what should we do here? I would prefer to avoid doing a diff on every render, because if anyone has a function as a prop, then it is likely to always trigger anyway and the diff would be useless. In most cases, setting options is essentially free (at least compared to a react render).

Maybe we can come up with some criteria for an early exit within setOptions

[Nantris]

I agree, it would be no good having the users who wouldn't benefit from this micro-optimization doing extra work for those who would.

Just spitballing here, but I wonder if it might be reasonable to do something along the lines of how immediatelyRender and shouldRerenderOnTransaction were introduced? I understand those were more about backwards compatibility and are really an escape hatch, but doing extra work at runtime is probably even more undesirable than introducing a new option.

It's a tiny bit better than the current workaround and has the benefit of then being documented.

An automatic solution is still preferrable but it doesn't jump out to me.

---

@yurtsiv I wonder how you noticed this in the first place? It's a good find! But I can't imagine it actually slowed anything down enough that you found it that way, right?

[yurtsiv]

There's another issue: pending state updates could change options and there is no way around in this scenario, editor options would have to be updated. Early returns (including option diffing) can only partially solve the problem.

I'm not sure how this can be fixed in the TipTap itself.

Besides dummy dependencies, there are two other solutions:

• Make sure transactions that trigger plugin view renders happen before React state updates.
• Use a flag on the view object to ensure the component is rendered only once.

---

@Nantris

I wonder how you noticed this in the first place?

After removing all dependencies from useEditor, one of the plugin views started acting strangely. I first noticed a flushSync error and began investigating. The issue was caused by two views being rendered while the system is designed to have only one at a time.

[nperez0111]

Hm, looking at this again, the silver lining here is that the functions are already optimized in a way that they only ever are registered on editor creation, and when re-applied, they reference the latest versions already.

tiptap/packages/react/src/useEditor.ts

Lines 133 to 147 in 7b4e6f5

	const optionsToApply: Partial<EditorOptions> = {
	...this.options.current,
	// Always call the most recent version of the callback function by default
	onBeforeCreate: (...args) => this.options.current.onBeforeCreate?.(...args),
	onBlur: (...args) => this.options.current.onBlur?.(...args),
	onCreate: (...args) => this.options.current.onCreate?.(...args),
	onDestroy: (...args) => this.options.current.onDestroy?.(...args),
	onFocus: (...args) => this.options.current.onFocus?.(...args),
	onSelectionUpdate: (...args) => this.options.current.onSelectionUpdate?.(...args),
	onTransaction: (...args) => this.options.current.onTransaction?.(...args),
	onUpdate: (...args) => this.options.current.onUpdate?.(...args),
	onContentError: (...args) => this.options.current.onContentError?.(...args),
	onDrop: (...args) => this.options.current.onDrop?.(...args),
	onPaste: (...args) => this.options.current.onPaste?.(...args),
	}

So, it looks like it would only be diffing some fairly basic values. Since the options object is rather small, I think it should be fine to just do a shallow equals between the keys & values of the known keys (i.e. explicitly excluding the functions so that it is more likely to actually work).

[nperez0111]

Here is what I came up with:

diff --git a/packages/react/src/useEditor.ts b/packages/react/src/useEditor.ts
index 776840615..2bba04fe0 100644
--- a/packages/react/src/useEditor.ts
+++ b/packages/react/src/useEditor.ts
@@ -197,13 +197,47 @@ class EditorInstanceManager {
       clearTimeout(this.scheduledDestructionTimeout)
 
       if (this.editor && !this.editor.isDestroyed && deps.length === 0) {
+        const editor = this.editor
+
         // if the editor does exist & deps are empty, we don't need to re-initialize the editor
-        // we can fast-path to update the editor options on the existing instance
-        this.editor.setOptions({
-          ...this.options.current,
-          editable: this.editor.isEditable,
-        })
+        if (Object.keys(this.options.current).some(key => {
+          if (['onCreate', 'onBeforeCreate', 'onDestroy', 'onUpdate', 'onTransaction', 'onFocus', 'onBlur', 'onSelectionUpdate', 'onContentError', 'onDrop', 'onPaste'].includes(key)) {
+            // we don't want to compare callbacks, they are always different and only registered once
+            return false
+          }
+
+          // We often encourage putting extensions inlined in the options object, so we will do a slightly deeper comparison here
+          if (key === 'extensions' && this.options.current.extensions) {
+            if (this.options.current.extensions.length !== editor.options.extensions.length) {
+              return true
+            }
+            return this.options.current.extensions.some((extension, index) => {
+              if (extension !== editor.options.extensions[index]) {
+                console.log('extensions changed', extension, editor.options.extensions[index])
+                return true
+              }
+              return false
+            })
+          }
+          if (this.options.current[key as keyof UseEditorOptions] !== editor.options[key as keyof EditorOptions]) {
+            console.log('options changed', key, this.options.current[key as keyof UseEditorOptions], editor.options[key as keyof EditorOptions])
+            // if any of the options have changed, we should update the editor options
+            return true
+          }
+          return false
+        })) {
+          console.count('fast-ish path')
+
+          // we can fast-path to update the editor options on the existing instance
+          this.editor.setOptions({
+            ...this.options.current,
+            editable: this.editor.isEditable,
+          })
+        } else {
+          console.count('fastest path')
+        }
       } else {
+        console.count('slowest path')
         // When the editor:
         // - does not yet exist
         // - is destroyed

[yurtsiv]

@nperez0111 content should also be excluded I think, some heavy strings could be in there

[nperez0111]

It's referential equality, so it would be a pointer compare ===, not a string compare, so no need to exclude it, and people definitely just change content and expect it to re-render.

A lot of React people use the controlled component pattern which is very inefficient, but I digress...

[nperez0111]

It will degrade to a string compare, if someone built a string that has a different pointer. At which point, shame on them, they deserve the performance hit 😂

[nperez0111]

We will see how it goes, #6031

[nperez0111]

@yurtsiv, would you mind validating the fix with this version please?

npm i https://pkg.pr.new/@tiptap/react@6031

From here: #6031 (comment)

[yurtsiv]

@nperez0111 I was thinking about the controlled component pattern. If you're fine with making it even less efficient, the proposed solution seems fine to me.

[nperez0111]

controlled component pattern is very harmful. React has trouble getting those to run even with basic input boxes, much less an entire rich text editor. I would very much advise you against it unless you want to solve performance issues later on.

It is much more performant to get the value from the editor instance when you actually need it rather than trying to synchronize it with some state. Like this:

DO NOT DO THIS:

function Component() {
  const [editorContent, setEditorContent] = React.useState(`
        <p>This isn’t bold.</p>
        <p><strong>This is bold.</strong></p>
      `)
  const editor = useEditor({
    extensions: [Document, Paragraph, Text, Bold],
    content: editorContent,
    onUpdate: ({ editor }) => {
      setEditorContent(editor.getHTML())
    },
  })

  return (
    <>
      {editorContent}
      <button onClick={() => saveToDB(editorContent)}>Send somewhere</button>
    </>
  )
}

DO THIS:

function Component() {
  const editor = useEditor({
    extensions: [Document, Paragraph, Text, Bold],
    content: editorContent,
    content: `
        <p>This isn’t bold.</p>
        <p><strong>This is bold.</strong></p>
      `,
  })

  return (
    <>
      {editor.getHTML()}
      <button onClick={() => saveToDB(editor.getHTML())}>Send somewhere</button>
    </>
  )
}

[yurtsiv]

Never tried that, was just thinking about other people out there. There probably aren’t many… 😄

[yurtsiv]

The patch is working 🎉 Had to memoize editorProps however. Perhaps worth mentioning in the documentation (same for parseOptions, coreExtensionOptions, and other complex options)

[Nantris]

Seems to work great here. All tests pass.

Nice patch @nperez0111!

[nperez0111]

Released in version 2.11.3

[nperez0111]

But, again, should probably reconsider your usage

[yurtsiv]

@nperez0111 You mean the usage of ReactRenderer inside plugin view?

[nperez0111]

I was referring to controlled component pattern.

But, now that you brought it up. Yea, using the ReactRenderer for a plugin view is definitely not a good pattern and nowhere do we recommend doing that. Plugin views are expected to be run synchronously, we put a lot of work to get it to work for Node Views, but honestly I believe it to be an anti-pattern anyway.

If you actually cared about performance or did not want to deal with subtle bugs with asynchronicity then I'd recommend you to just write the plain JS to create whatever view you are trying to make

[nperez0111]

I fixed this issue more out of the potential perf benefits rather than actually endorsing this sort of usage.

[yurtsiv]

Writing the view in plain JS is not feasible in my case. There're no performance issues however, it's rendered only once and floats around on each transaction.

As for the controlled component pattern, I don't use it.

Thanks for all the help, I appreciate that!