Skip to content

Commit

Permalink
[editable] Fix Memory Leak when switching between focused editables (#…
Browse files Browse the repository at this point in the history
…5542)

* [editable] Fix Memory Leak when switching between focused editable containers

I've found a memory leak in Slate React where-in when you'd switch between two pages, each with their own editor instance, auto-focusing on these elements causes a memory leak, where there would be a lot of Detached HTML Element's just floating around.

At first I thought it a bunch of different bugs with chromium, but noticed that when I force removed the input elements from the dom before the component would unmount, we would still see small leak referencing "latestElement" (while simultaniously, I would still the element & children references in the `IN_FOCUSE` WeakMap) Looking at editable's code, I realized that we're storing state in a weird way, directly mutating it using `useMemo`, and React isn't removing all references (probably because its still stored in the WeakMap.

The simple fix for this is what I've commited; using `useLayoutEffect`, we forcably remove the `latestElement` and references to it.

* Update editable.tsx

* Add changeset

---------

Co-authored-by: Dylan Schiemann <dylan@dojotoolkit.org>
  • Loading branch information
hellsan631 and dylans authored Oct 31, 2023
1 parent 3b41384 commit 8688ed5
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 0 deletions.
5 changes: 5 additions & 0 deletions .changeset/empty-shirts-mate.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'slate-react': patch
---

Fix Memory Leak when switching between focused editables
16 changes: 16 additions & 0 deletions packages/slate-react/src/components/editable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import throttle from 'lodash/throttle'
import React, {
useCallback,
useEffect,
useLayoutEffect,
useMemo,
useReducer,
useRef,
Expand Down Expand Up @@ -176,6 +177,21 @@ export const Editable = (props: EditableProps) => {
[]
)

useLayoutEffect(() => {
return () => {
if (state == null) {
return
}
// Avoid leaking DOM nodes when this component is unmounted.
if (state.latestElement != null) {
state.latestElement.remove()
}
if (state.latestElement != null) {
state.latestElement = null
}
}
}, [])

Check warning on line 193 in packages/slate-react/src/components/editable.tsx

View workflow job for this annotation

GitHub Actions / latest

React Hook useLayoutEffect has a missing dependency: 'state'. Either include it or remove the dependency array

Check warning on line 193 in packages/slate-react/src/components/editable.tsx

View workflow job for this annotation

GitHub Actions / dev

React Hook useLayoutEffect has a missing dependency: 'state'. Either include it or remove the dependency array

Check warning on line 193 in packages/slate-react/src/components/editable.tsx

View workflow job for this annotation

GitHub Actions / lint:eslint

React Hook useLayoutEffect has a missing dependency: 'state'. Either include it or remove the dependency array

// The autoFocus TextareaHTMLAttribute doesn't do anything on a div, so it
// needs to be manually focused.
useEffect(() => {
Expand Down

0 comments on commit 8688ed5

Please sign in to comment.