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.

Sign up for GitHub

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

Jump to bottom

fix(react): use ref instead of state in useEditor to prevent rerenders #4856

Merged
merged 1 commit into from
Feb 6, 2024
Merged

fix(react): use ref instead of state in useEditor to prevent rerenders #4856

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 34 additions & 35 deletions packages/react/src/useEditor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,9 @@ import {

import { Editor } from './Editor.js'

function useForceUpdate() {
const [, setValue] = useState(0)

return () => setValue(value => value + 1)
}

export const useEditor = (options: Partial<EditorOptions> = {}, deps: DependencyList = []) => {
const [editor, setEditor] = useState<Editor | null>(null)

const forceUpdate = useForceUpdate()
const editorRef = useRef<Editor | null>(null)
const [, forceUpdate] = useState({})

const {
onBeforeCreate,
Expand All @@ -42,71 +35,77 @@ export const useEditor = (options: Partial<EditorOptions> = {}, deps: Dependency
// This effect will handle updating the editor instance
// when the event handlers change.
useEffect(() => {
if (!editor) {
if (!editorRef.current) {
return
}

if (onBeforeCreate) {
editor.off('beforeCreate', onBeforeCreateRef.current)
editor.on('beforeCreate', onBeforeCreate)
editorRef.current.off('beforeCreate', onBeforeCreateRef.current)
editorRef.current.on('beforeCreate', onBeforeCreate)

onBeforeCreateRef.current = onBeforeCreate
}

if (onBlur) {
editor.off('blur', onBlurRef.current)
editor.on('blur', onBlur)
editorRef.current.off('blur', onBlurRef.current)
editorRef.current.on('blur', onBlur)

onBlurRef.current = onBlur
}

if (onCreate) {
editor.off('create', onCreateRef.current)
editor.on('create', onCreate)
editorRef.current.off('create', onCreateRef.current)
editorRef.current.on('create', onCreate)

onCreateRef.current = onCreate
}

if (onDestroy) {
editor.off('destroy', onDestroyRef.current)
editor.on('destroy', onDestroy)
editorRef.current.off('destroy', onDestroyRef.current)
editorRef.current.on('destroy', onDestroy)

onDestroyRef.current = onDestroy
}

if (onFocus) {
editor.off('focus', onFocusRef.current)
editor.on('focus', onFocus)
editorRef.current.off('focus', onFocusRef.current)
editorRef.current.on('focus', onFocus)

onFocusRef.current = onFocus
}

if (onSelectionUpdate) {
editor.off('selectionUpdate', onSelectionUpdateRef.current)
editor.on('selectionUpdate', onSelectionUpdate)
editorRef.current.off('selectionUpdate', onSelectionUpdateRef.current)
editorRef.current.on('selectionUpdate', onSelectionUpdate)

onSelectionUpdateRef.current = onSelectionUpdate
}

if (onTransaction) {
editor.off('transaction', onTransactionRef.current)
editor.on('transaction', onTransaction)
editorRef.current.off('transaction', onTransactionRef.current)
editorRef.current.on('transaction', onTransaction)

onTransactionRef.current = onTransaction
}

if (onUpdate) {
editor.off('update', onUpdateRef.current)
editor.on('update', onUpdate)
editorRef.current.off('update', onUpdateRef.current)
editorRef.current.on('update', onUpdate)

onUpdateRef.current = onUpdate
}
}, [onBeforeCreate, onBlur, onCreate, onDestroy, onFocus, onSelectionUpdate, onTransaction, onUpdate, editor])
}, [onBeforeCreate, onBlur, onCreate, onDestroy, onFocus, onSelectionUpdate, onTransaction, onUpdate, editorRef.current])

useEffect(() => {
let isMounted = true

const instance = new Editor(options)

setEditor(instance)
editorRef.current = new Editor(options)

instance.on('transaction', () => {
editorRef.current.on('transaction', () => {
requestAnimationFrame(() => {
requestAnimationFrame(() => {
if (isMounted) {
forceUpdate()
forceUpdate({})
}
})
})
Expand All @@ -119,9 +118,9 @@ export const useEditor = (options: Partial<EditorOptions> = {}, deps: Dependency

useEffect(() => {
return () => {
editor?.destroy()
return editorRef.current?.destroy()
}
}, [editor])
}, [])
Comment on lines 119 to +123
Copy link
Contributor

@sjdemartini sjdemartini Feb 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@svenadlung Won't this new logic lead to memory leaks (and potentially other negative side effects)? As of this PR, whenever deps change, a new Editor() is created. However, this code now only destroys the editor on final unmount. So it will not be calling destroy on any intermediary editors that are created as deps change.

Maybe this can be fixed by moving the destroy() call to within the cleanup callback for the useEffect above that creates the editor. Something like

  useEffect(() => {
    let isMounted = true

    editorRef.current = new Editor(options)

    editorRef.current.on('transaction', () => {
      requestAnimationFrame(() => {
        requestAnimationFrame(() => {
          if (isMounted) {
            forceUpdate({})
          }
        })
      })
    })
    return () => {
      isMounted = false
      editorRef.current?.destroy();
    }
  }, deps)

Similarly, using editorRef.current as a dependency for useEffect for changing event handlers will not work (see explanation here https://stackoverflow.com/a/60476525/4543977). This means that if the editor changes as deps change, it will seemingly not be updating any of the callbacks on the new editor instance. So each new editor instance will not be set up correctly.

I see the comment in the linked issue #4482 (comment) referencing 6984ea1 as being problematic. Seems that "Destroy editor in safe" PR #4000 was potentially the incorrect solution to its bug, and I think similarly this is a bandaid to attempt to fix a new bug that PR introduced, but unfortunately will probably introduce new bugs itself. 😕 (For what it's worth, I am still on 2.0.3 and do not experience the [Bug]: Collaboration Cursor flickering issue.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eye @sjdemartini. I'm not sure exactly what .destroy entails but on the surface this concern appears to be valid.

On the other hand, it seems possible any loose references will be dropped as they become inaccessible via any code paths - but I think for event listeners this could still be a problem, right?

As far as I know destroy only affects the view layer, so is it possible/safe to destroy the old editor before creating the new one? Or would that obliterate history and whatnot? It seems like it would disrupt any decoration state at the very least.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the very least, the new merged implementation will be buggy in that that the event-handlers will not be reassigned whenever deps change for useEditor. So new editor instances will not function properly as dependencies change, which is most certainly a bug.

But I think it's likely the negative effects will be worse than that, since destroy isn't called (event listeners not unassigned from stale versions of the Editor, etc.).

While this new release may fix the flickering with the collaboration cursor, I think that bug should be resolved by going back to what introduced it. It's probably worth looking at the implementation of useEditor prior to tiptap 2.1.0, where the flicker was not present.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tried 2.2.x yet due to other blockers, but I certainly hope that memory leaks won't be introduced to accommodate features we don't use.

It stands to reason that the implementation leaks, but have you done any testing to try to determine if there are indeed memory leaks @sjdemartini?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested this yet and unfortunately am unlikely to have time, as I'm swamped right now. I'm planning to stay on v2.0.* for now, based on the new issues (flickering with the collaboration extension in 2.1, and the new problems here around listeners and/or memory problems in 2.2).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

For reference, there won't be any memory leak if you keep the dependency array empty.

I would remove the deps array altogether because I don't think anyone would intentionally want to re-create the editor.

Copy link
Contributor

@sjdemartini sjdemartini Feb 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but that's my whole point: if you actually do need to use the dependency array, this new code will not work properly. The dependency array exists for a reason and has since the early days of Tiptap, as there are legitimate use-cases that require recreating the editor when dependencies change (vs just using methods on the editor itself or something). For instance, you may need to do so if your list of extensions changes.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe your assumption has been right @sjdemartini. I've noticed a breaking change going from 2.2.1 (prior to this change) and 2.2.2 (the release of this change). If you call editor.commands.focus() from the onCreate handler it will work in 2.2.1 and not in 2.2.2. Likely because the handlers haven't been reassigned after a new editor instance was created.

Here's two codesandboxes comparing the two versions:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samvantoever can you check whether adding a setTimeout around your focus call resolves the issue? If so, I would think that suggests race condition rather than handler assignment issue.


return editor
return editorRef.current
}
Loading