ueberdosis / tiptap

The headless rich text editor framework for web artisans.
https://tiptap.dev
MIT License
26.54k stars 2.23k forks source link

flushSync gets called on render when the Editor gets updated with content that uses an extension with addNodeView #3764

Closed bozhi closed 1 year ago

bozhi commented 1 year ago

What’s the bug you are facing?

I noticed that flushSync gets called on render when editorContent gets updated with content that uses addNodeView. This results in the warning message: Warning: flushSync was called from inside a lifecycle method. React cannot flush when React is already rendering. Consider moving this call to a scheduler task or micro task.

Which browser was this experienced in? Are any special extensions installed?

Which browser was this experienced in? Google Chrome

Are any special extensions installed? I have copied the demo extension from "https://github.com/ueberdosis/tiptap/blob/main/demos/src/Examples/InteractivityComponentContent/React/Extension.js" to the "App.js" file in the CodeSandbox.io example to illustrated the bug.

How can we reproduce the bug on our side?

To recreate manually 1) Create an extension that utilizes addNodeView as part of the render 2) Add content that renders the extension to the editorcontent.content after the first render. To do this, I used router.isReady to trigger the re-render by setting the content data after the router is ready

Sample code

import {
  EditorContent,
  useEditor,
  ReactNodeViewRenderer,
  NodeViewContent,
  NodeViewWrapper,
} from "@tiptap/react";
import { useRouter } from "next/router";
import { useEffect, useState } from "react";
import StarterKit from "@tiptap/starter-kit";

import React from "react";

import { mergeAttributes, Node } from "@tiptap/core";

const Component = () => {
  return (
    <NodeViewWrapper className="react-component-with-content">
      <span className="label" contentEditable={false}>
        React Component
      </span>

      <NodeViewContent className="content" />
    </NodeViewWrapper>
  );
};

const Extension = Node.create({
  name: "reactComponent",

  group: "block",

  content: "inline*",

  parseHTML() {
    return [
      {
        tag: "react-component",
      },
    ];
  },

  renderHTML({ HTMLAttributes }) {
    return ["react-component", mergeAttributes(HTMLAttributes), 0];
  },

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

const TiptapTest = ({ data }: { data: string }) => {
  const editor = useEditor(
    {
      extensions: [StarterKit, Extension],
      content: data,
    },
    [data]
  );

  return (
    <div>
      <EditorContent editor={editor} />
    </div>
  );
};
export default function Index() {
  const router = useRouter();
  const [data, setData] = useState("");
  useEffect(() => {
    if (router.query) {
      setData(`<p>
      This is still the text editor you’re used to, but enriched with node views.
    </p>
    <react-component>
      <p>This is editable.</p>
    </react-component>
    <p>
      Did you see that? That’s a React component. We are really living in the future.
    </p>`);
    }
  }, [router.isReady]);

  return TiptapTest({ data });
}

Can you provide a CodeSandbox?

https://codesandbox.io/s/modern-sun-h7r3kg

What did you expect to happen?

flushSync to not be called during render.

Anything to add? (optional)

I suspect that the problem is caused when flushSync gets called from inside a lifecycle method when rendering content that has extensions that utilizes addNodeView when the editor unmounts and re-mounts.

In fix #3533, flushSync was queued onto the microtask in order to prevent flushSync from being called from a lifecycle method. This does not account for what happens when the editor un-mounts and re-mounts and after editorContent.initialized is set to "true".

We propose a modification to EditorContent.tsx that unsets the initialized state when the component is about to be unmounted:

componentWillUnmount() {
    this.initialized = false;
}

Did you update your dependencies?

Are you sponsoring us?

vishaltelangre commented 1 year ago

This is reoccurring in the latest v2.0.1. I have forked the above CodeSandbox example and updated the tiptap dependencies to reproduce it. See https://codesandbox.io/s/stupefied-violet-0r9z6l?file=/src/App.js.

It wasn't an issue for us in v2.0.0-beta.220 but after upgrading to the latest v2.0.1, it has started resurfacing again.

Cc @KentoMoriwaki, @bdbch

holdenmatt commented 1 year ago

Should this bug be re-opened? I'm still seeing it in tiptap v2.0.3.

To original CodeSandbox works as a repro after upgrading the tiptap dependencies.

jonahallibone commented 1 year ago

+1 also get this in v2.0.3

cpoonolly commented 1 year ago

Saw this as well in v2.0.2

KentoMoriwaki commented 1 year ago

This PR would help with this issue! https://github.com/ueberdosis/tiptap/pull/4000

holdenmatt commented 1 year ago

Has anyone found a workaround for this issue? I'm prepping a product for launch, and it's a pretty nasty bug. I sporadically see the flushSync error when unmounting/remounting the editor with different content, and it crashes the app, requiring a page reload.

Any idea how to prevent this?

reza-akbari commented 1 year ago

it's not a real fix but this got rid of the error message for me:

instead of doing this:

useEffect(() => {

  editor.commands.setContent(content);

}, [content]);

I did this:

useEffect(() => {

  setTimeout(() => {
    editor.commands.setContent(content);
  });

}, [content]);

( but of course I'm not using the content option initially and my editor doesn't get destroyed on route change )

KentoMoriwaki commented 1 year ago

My pull request #4000 will resolve the warning that occurs when recreating the editor instance. However, it will not address the warning that occurs when perform editor.commands within useEffect. In fact, that issue is fundamentally a correct warning and needs to be moved to queueMicrotask or setTimeout as stated in the warning.

To perform editor.commands, which means updating the state and view of ProseMirror, it is necessary to mutate then DOM synchronously within that process . This is essential for fully synchronizing the node and selection with the DOM in ProseMirror. Therefore, executing flushSync is indispensable.

When perform editor.commands within useEffect, the changes to the DOM cannot be immediately applied even if flushSync is executed because React is already in the render phase (this is a limitation in React's design). This may cause unexpected bugs by disrupting the synchronization between ProseMirror's state and the DOM.

So, when performing editor.commands within useEffect, as mentioned above https://github.com/ueberdosis/tiptap/issues/3764#issuecomment-1546629928, it is the correct solution to wrap it with setTimeout or queueMicrotask.

holdenmatt commented 1 year ago

I was previously setting content in useEditor. Using editor.commands.setContent instead (wrapped in a setTimeout) as mentioned in the previous comments fixed this issue for me. Thanks for the helpful suggestions!

nk11dev commented 1 year ago

I have this issue too and wrapping in setTimeout did not help - Warning: flushSync... still appears. Even if it worked, this could be just temporary fix. This bug should be fixed in the package itself.

Please, take a look at the PR https://github.com/ueberdosis/tiptap/pull/4000

rbruels commented 1 year ago

Noting here as well (cc @bdbch and #3580), still happening in 2.0.4. Code sandbox: https://codesandbox.io/s/github/rbruels/tiptap-3580-sandbox

bdbch commented 1 year ago

added this back into the backlog

bdbch commented 1 year ago

Nvm - closing this as it seem to be a duplicate of #3580