-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
fix(react): use ref instead of state in useEditor to prevent rerenders #4856
Conversation
✅ Deploy Preview for tiptap-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Happy to see this change. Thanks for all the work @svenadlung and @bdbch. |
It would be nice if we could do something about the first |
Could you clarify a bit more which null render you mean? |
@rfgamaral I will create a new PR based on this one, once it is merged. |
@rfgamaral sorry, we didn't want to block this bugfix and released 2.2.2. We are currently looking at the other issue. |
@svenadlung No worries, thanks for looking into it 👍 |
useEffect(() => { | ||
return () => { | ||
editor?.destroy() | ||
return editorRef.current?.destroy() | ||
} | ||
}, [editor]) | ||
}, []) |
There was a problem hiding this comment.
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 destroy
s the editor
on final unmount. So it will not be calling destroy
on any intermediary editor
s 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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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.
Big love for getting this merged ❤️ I hit this issue today and was about to do the manual fix from #4482 but was pleased to see this was released yesterday 🎉 |
Sorry, I misunderstood that. Glad it works for you :) |
Ref
Thx @prichodko
To be done …
Please describe your changes
[add a description of your changes here]
How did you accomplish your changes
[add a detailed description of how you accomplished your changes here]
How have you tested your changes
[add a detailed description of how you tested your changes here]
How can we verify your changes
[add a detailed description of how we can verify your changes here]
Remarks
[add any additional remarks here]
Checklist
Related issues
[add a link to the related issues here]