Secondary Suspense causing view to be prematurely destroyed

[ariellebryn]

In exploring using suspense on a page that has a react-prosemirror field, I'm running into an error when: the page loads, then suspends for some reason, then un-suspends.

This seems to be what's happening:

• The page loads, including the <ProseMirror> component
• The page suspends for some reason (in my case, I was triggering a refetch and had put the Suspense boundary at a higher level)
  • When React suspends, it calls the cleanup function of layout effects! In their words: "If React needs to hide the already visible content because it suspended again, it will clean up layout Effects in the content tree. When the content is ready to be shown again, React will fire the layout Effects again. This ensures that Effects measuring the DOM layout don’t try to do this while the content is hidden."
• This causes the cleanup function of this useLayoutEffect to run: view.destroy();, even though the component isn't actually unmounting
• When suspense ends and the component is re-rendered (with the rest of its state maintained), it still has access to the EditorView (that had destroy() called on it), but now that view is missing key fields!

[smoores-dev]

So I can think of two approaches to this. The first feels kind of clunky: we could completely replace the view in the useLayoutEffect if it has been destroyed (which we can determine by inspecting the docView property, which gets set to null in EditorView.destroy()). Something like this:

  const directEditorPropsRef = useRef(directEditorProps);
  directEditorPropsRef.current = directEditorProps;

  const mountRef = useRef(mount);
  mountRef.current = mount;

  const [view, setView] = useState<EditorView | null>(null);

  useLayoutEffect(() => {
    if (view && !view.docView) {
      if (mountRef.current) {
        setView(
          new EditorView(
            { mount: mountRef.current },
            directEditorPropsRef.current
          )
        );
      }
    }
    return () => {
      if (view) {
        view.destroy();
      }
    };
  }, [view]);

The second option would be to do the view.destroy() in a useEffect cleanup, rather than a useLayoutEffect cleanup. I'm not sure whether this would be correct or not... This would mean that, during a suspense, the ProseMirror event handlers and plugin views would still be active. This feels a little odd, but as far as I know, installing document-level event listeners in a useEffect is common practice in React, and presumably this is expected behavior with Suspense

[VictoriaOtm]

Hi!
I caught the same issue in my project, we noticed it with Sentry error "TypeError
Cannot read properties of null (reading 'matchesNode')" which is about the line https://github.com/ProseMirror/prosemirror-view/blob/17b508f618c944c54776f8ddac45edcb49970796/src/index.ts#L181

I went through the stack trace and saw the same problem which was described by @ariellebryn (we also use Suspense).

@smoores-dev I think both of your ideas are ok, the second one looks a bit more reliable but it's needed to be tested. Btw why do you call view.destroy() in useLayoutEffect now? I suppose there was a reason

Anyways, I'll be happy if you release the fix, thanks!

[smoores-dev]

We call view.destroy in a useLayoutEffect cleanup function to ensure that if the EditorView is unmounted, the ProseMirror DOM is removed before the user sees the repaint. In the mainline version of this project, ProseMirror still owns the DOM related to the editor, so we need to actively tell it to clean that DOM up when the component it belongs to is being unmounted.

The issue here is that Suspense calls the layout effect cleanups without unmounting the component. Looking closer though, I'm increasingly feeling like the underlying problem here is actually that we don't set the editor view to null in a cleanup function; we do it in the effect itself. I think this is wrong, is probably separately an issue, and if we fixed it, would also fix this issue. I'll see if I can open a PR for this today!