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

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

Closed
1 of 2 tasks
bozhi opened this issue Feb 20, 2023 · 13 comments · Fixed by #3781 or #4000
Closed
1 of 2 tasks
Labels
Info: Duplicate The issue or pullrequest is a duplicate of another issue or pullrequest Type: Bug The issue or pullrequest is related to a bug

Comments

@bozhi
Copy link

bozhi commented Feb 20, 2023

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?

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

Are you sponsoring us?

  • Yes, I’m a sponsor. 💖
@bozhi bozhi added the Type: Bug The issue or pullrequest is related to a bug label Feb 20, 2023
@vishaltelangre
Copy link

vishaltelangre commented Apr 3, 2023

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
Copy link

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
Copy link

+1 also get this in v2.0.3

@cpoonolly
Copy link

Saw this as well in v2.0.2

@bdbch bdbch reopened this Apr 19, 2023
@KentoMoriwaki
Copy link
Contributor

This PR would help with this issue! #4000

@holdenmatt
Copy link

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
Copy link

reza-akbari commented May 13, 2023

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
Copy link
Contributor

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 #3764 (comment), it is the correct solution to wrap it with setTimeout or queueMicrotask.

@holdenmatt
Copy link

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
Copy link

nk11dev commented May 25, 2023

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 #4000

@rbruels
Copy link

rbruels commented Aug 4, 2023

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 bdbch reopened this Aug 5, 2023
@bdbch
Copy link
Contributor

bdbch commented Aug 5, 2023

added this back into the backlog

@bdbch
Copy link
Contributor

bdbch commented Aug 5, 2023

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

@bdbch bdbch closed this as completed Aug 5, 2023
@bdbch bdbch added the Info: Duplicate The issue or pullrequest is a duplicate of another issue or pullrequest label Aug 5, 2023
baiirun added a commit to geobrowser/geogenesis that referenced this issue Aug 28, 2023
baiirun added a commit to geobrowser/geogenesis that referenced this issue Aug 28, 2023
…editor (#477)

* fix: tiptap might cause runtime react/DOM errors when remounting the editor

See: ueberdosis/tiptap#3764 (comment)

* update tiptap deps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Info: Duplicate The issue or pullrequest is a duplicate of another issue or pullrequest Type: Bug The issue or pullrequest is related to a bug
Projects
Status: Done
10 participants