Fix primefaces#5927: BodyCell memory leak when not editable

melloware · Feb 20, 2024 · bd0d1b3 · bd0d1b3
1 parent 69b9adc
commit bd0d1b3
Show file tree

Hide file tree

Showing 3 changed files with 62 additions and 31 deletions.
diff --git a/components/lib/hooks/useEventListener.js b/components/lib/hooks/useEventListener.js
@@ -1,4 +1,3 @@
-/* eslint-disable */
 import * as React from 'react';
 import { DomHandler, ObjectUtils } from '../utils/Utils';
 import { usePrevious } from './usePrevious';
@@ -11,9 +10,11 @@ export const useEventListener = ({ target = 'document', type, listener, options,
     let prevOptions = usePrevious(options);
 
     const bind = (bindOptions = {}) => {
-        if (ObjectUtils.isNotEmpty(bindOptions.target)) {
+        const { target: bindTarget } = bindOptions;
+
+        if (ObjectUtils.isNotEmpty(bindTarget)) {
             unbind();
-            (bindOptions.when || when) && (targetRef.current = DomHandler.getTargetElement(bindOptions.target));
+            (bindOptions.when || when) && (targetRef.current = DomHandler.getTargetElement(bindTarget));
         }
 
         if (!listenerRef.current && targetRef.current) {
@@ -29,32 +30,45 @@ export const useEventListener = ({ target = 'document', type, listener, options,
         }
     };
 
-    React.useEffect(() => {
+    const dispose = () => {
+        unbind();
+        // Prevent memory leak by releasing
+        listenerRef.current = null;
+        prevListener = null;
+        prevOptions = null;
+    };
+
+    const updateTarget = React.useCallback(() => {
         if (when) {
             targetRef.current = DomHandler.getTargetElement(target);
         } else {
             unbind();
             targetRef.current = null;
         }
+        // eslint-disable-next-line react-hooks/exhaustive-deps
     }, [target, when]);
 
     React.useEffect(() => {
-        // to properly compare functions we can implicitly converting the function to it's body's text as a String
-        if (listenerRef.current && ('' + prevListener !== '' + listener || prevOptions !== options)) {
+        updateTarget();
+    }, [updateTarget]);
+
+    React.useEffect(() => {
+        const listenerChanged = `${prevListener}` !== `${listener}`;
+        const optionsChanged = prevOptions !== options;
+        const listenerExists = listenerRef.current;
+
+        if (listenerExists && (listenerChanged || optionsChanged)) {
             unbind();
             when && bind();
+        } else if (!when || !listenerExists) {
+            dispose();
         }
-        return () => {
-            // #5927 prevent memory leak by releasing
-            prevListener = null;
-            prevOptions = null;
-        };
+        // eslint-disable-next-line react-hooks/exhaustive-deps
     }, [listener, options, when]);
 
     useUnmountEffect(() => {
-        unbind();
+        dispose();
     });
 
     return [bind, unbind];
 };
-/* eslint-enable */
diff --git a/components/lib/hooks/useOverlayScrollListener.js b/components/lib/hooks/useOverlayScrollListener.js
@@ -1,15 +1,14 @@
-/* eslint-disable */
 import * as React from 'react';
 import PrimeReact, { PrimeReactContext } from '../api/Api';
 import { DomHandler, ObjectUtils } from '../utils/Utils';
 import { usePrevious } from './usePrevious';
 import { useUnmountEffect } from './useUnmountEffect';
 
 export const useOverlayScrollListener = ({ target, listener, options, when = true }) => {
+    const context = React.useContext(PrimeReactContext);
     const targetRef = React.useRef(null);
     const listenerRef = React.useRef(null);
-    const scrollableParents = React.useRef([]);
-    const context = React.useContext(PrimeReactContext);
+    const scrollableParentsRef = React.useRef([]);
     let prevListener = usePrevious(listener);
     let prevOptions = usePrevious(options);
 
@@ -21,7 +20,7 @@ export const useOverlayScrollListener = ({ target, listener, options, when = tru
 
         if (!listenerRef.current && targetRef.current) {
             const hideOnScroll = context ? context.hideOverlaysOnDocumentScrolling : PrimeReact.hideOverlaysOnDocumentScrolling;
-            const nodes = (scrollableParents.current = DomHandler.getScrollableParents(targetRef.current, hideOnScroll));
+            const nodes = (scrollableParentsRef.current = DomHandler.getScrollableParents(targetRef.current, hideOnScroll));
 
             listenerRef.current = (event) => listener && listener(event);
             nodes.forEach((node) => node.addEventListener('scroll', listenerRef.current, options));
@@ -30,39 +29,53 @@ export const useOverlayScrollListener = ({ target, listener, options, when = tru
 
     const unbind = () => {
         if (listenerRef.current) {
-            const nodes = scrollableParents.current;
+            const nodes = scrollableParentsRef.current;
+
             nodes.forEach((node) => node.removeEventListener('scroll', listenerRef.current, options));
 
             listenerRef.current = null;
         }
     };
 
-    React.useEffect(() => {
+    const dispose = () => {
+        unbind();
+        // #5927 prevent memory leak by releasing
+        scrollableParentsRef.current = null;
+        prevListener = null;
+        prevOptions = null;
+    };
+
+    const updateTarget = React.useCallback(() => {
         if (when) {
             targetRef.current = DomHandler.getTargetElement(target);
         } else {
             unbind();
             targetRef.current = null;
         }
+        // eslint-disable-next-line react-hooks/exhaustive-deps
     }, [target, when]);
 
     React.useEffect(() => {
-        // to properly compare functions we can implicitly converting the function to it's body's text as a String
-        if (listenerRef.current && ('' + prevListener !== '' + listener || prevOptions !== options)) {
+        updateTarget();
+    }, [updateTarget]);
+
+    React.useEffect(() => {
+        const listenerChanged = `${prevListener}` !== `${listener}`;
+        const optionsChanged = prevOptions !== options;
+        const listenerExists = listenerRef.current;
+
+        if (listenerExists && (listenerChanged || optionsChanged)) {
             unbind();
             when && bind();
+        } else if (!when || !listenerExists) {
+            dispose();
         }
-        return () => {
-            // #5927 prevent memory leak by releasing
-            prevListener = null;
-            prevOptions = null;
-        };
-    }, [listener, options]);
+        // eslint-disable-next-line react-hooks/exhaustive-deps
+    }, [listener, options, when]);
 
     useUnmountEffect(() => {
-        unbind();
+        dispose();
     });
 
     return [bind, unbind];
 };
-/* eslint-enable */
diff --git a/components/lib/hooks/usePrevious.js b/components/lib/hooks/usePrevious.js
@@ -1,11 +1,15 @@
 import * as React from 'react';
 
 export const usePrevious = (newValue) => {
-    const ref = React.useRef(undefined);
+    const ref = React.useRef(null);
 
     React.useEffect(() => {
         ref.current = newValue;
-    });
+
+        return () => {
+            ref.current = null;
+        };
+    }, [newValue]);
 
     return ref.current;
 };