[Bug]: useEditor always creates at least 2 editors on mount

[schontz]

Affected Packages

react

Version(s)

develop

Bug Description

The editor is destroyed and re-created whenever deps changes. But this useEffect will run once on mount to start, before deps has actually changed. If deps are supplied, then the editor created in useState will get destroyed immediately in useEffect and get re-created.

It may be generally useful to have a hook that only runs when things change, e.g.:

function useChangeEffect(effect, deps) {
  const isFirstTime = useRef(true);
  useEffect(() => {
    if (isFirstTime.current) isFirstTime.current = false;
    else return effect();
  }, deps);
}

Then you can just replace this useEffect with useChangeEffect.

On a related note, applying options instead of re-creating an editor is useless code, because the effect only runs when deps change, not when options change. That means updated options are never injected into the editor. Furthermore, mostRecentOptions is a misnomer, because it is not actually updated on each render, so it is effectively useless.

Browser Used

Chrome

Code Example URL

No response

Expected Behavior

Editor is only created once if deps do not change.

Additional Context (Optional)

No response

Dependency Updates

• Yes, I've updated all my dependencies.

[nperez0111]

Thanks for bringing it up, I was a bit too focused on another bug. I'll consider your some of your suggestions.

[schontz]

Looking at useEditor further, it doesn't even look like deleteUnusedEditor will ever work, because mostRecentEditor.current will always point to the value of editor.

1. Create editor via useState
2. useEffect will destroy right away and re-create on mount (this bug)
3. If deps changes, deleteUnusedEditor will run again, but the editor is current
4. On unmount, deleteUnusedEditor will run, but it will not destroy the editor because editorInstance is the most recent editor

I was working on a PR for this, but there are multiple issues with the logic and I worry a little about pulling the thread.

I'm not clear under what conditions useEffect cleanup will run before the editor has been replaced, since it will be run after deps change (which necessitates a new editor).

The change that introduced this was supposedly for a strict mode issue, but because we create the editor via useState, it will always be run 2x in strict mode. There's no real workaround, so that means an orphaned editor will be created on every mount in strict mode. I know people want immediate rendering to work, and perhaps we can bring this up with React core team, as I have tried may workarounds for this type of scenario, but I have yet to find a practical solution. I suppose leaking memory in dev isn't the end of the world, but it's still frustrating.

That said, it is more react-y if it is not available on sync and instead runs all the commands via a useEffect. For example this bug wants to focus the editor right away, but in React the DOM isn't available until effects run, so the prescribed method should be something like:

const editor = useEditor(...)

useEffect(() => {
  if (!editor) return;
  editor.commands.focus();
}, [editor]);

// or via onCreate
const editor = useEditor({
  onCreate: ({ editor }) => editor.commands.focus()
})

Although there are scenarios when the editor gets re-created, which would call that effect multiple times. This could be solved via an option, though a properly descriptive name is hard to come by:

const editor = useEditor({
  onFirstTimeCreate: ({ editor }) => editor.commands.focus()
})

There could be more discussion around the API as a whole. For example, useEditor could return a ref instead of a state, assuming it is available on mount:

const editorRef = useEditor(...)

useEffect(() => {
  // editorRef.current is set, much like DOM refs
  const editor = editorRef.current;
  if (!editor) return;
  editor.commands.focus();
}, [])

I know that doesn't play well with deps, as the editor ref may change over time. One might argue that the actual editor instance changing should not be something that you need to react to (assuming state is preserved between instances), and instead the editor should only be a ref that is reacted to via useEditorState.

I put together a little StackBlitz Sample that shows some of these issues with a fake editor in strict and non strict modes. You can see that in both modes, the timeout destroy is not running on dismount.

[schontz]

Note that I am able to apply some of the patches locally and it stops creating new editors, though we'll have to refactor the "useState for new editor" if we really want things to be safe in React land.

[nperez0111]

Here is my stab at it: #5445

I think the crux of the change was to have the effect run on every render (without a dep array) and to schedule destruction of instances, but bail on the actual destruction if the instance was still mounted and nothing else changed

[fuadsaud]

Cross-posting here for visibility: #5414 (comment)

[rfgamaral]

@nperez0111 Did this issue not get fixed with #5445? Is there a reason why the issue remains open?

[nperez0111]

Working on releasing it now actually 😅

My CI has a bug that kept trying to release it as a minor that I was fighting. Will get it out as a patch

[nperez0111]

This is now released with v2.5.9!