ResizeObserver Behavior Fixes and Changes

Updated the useResizeObserver to use `target` instead of `getTarget`. Stopped using the "shared" ResizeObserver for additionl performance since it lead to memory leaks when components unmounted. For some reason, the `observer.unobserve` doesn't actually stop the handlers from being called if it happened during a React unmount phase? The only way I could prevent the memory leak warnings was to isolate the resize observer to each useEffect and use `observer.disconnect()` which stops *all** (1) observered targets. Using `observer.unobserve` would still cause the memory leak warnings. The downside to this is that it is less performant, but this was probably a pre-mature optimiziation anyways. WICG/resize-observer#59
mlaursen · Nov 11, 2019 · 6057fa7 · 6057fa7
1 parent 3f52c77
commit 6057fa7
Show file tree

Hide file tree

Showing 6 changed files with 63 additions and 132 deletions.
diff --git a/packages/documentation/src/components/AppBarTitle.tsx b/packages/documentation/src/components/AppBarTitle.tsx
@@ -38,7 +38,7 @@ const AppBarTitle: FC<AppBarTitleProps> = ({
         setTooltip(null);
       }
     },
-    getTarget: ref,
+    target: ref,
   });
 
   const id = useMemo(() => {

diff --git a/packages/form/src/text-field/TextArea.tsx b/packages/form/src/text-field/TextArea.tsx
@@ -213,7 +213,7 @@ const TextArea: FC<TextAreaProps & WithRef> = providedProps => {
   // recalculate the height when the width changes as well.
   useResizeObserver({
     disableHeight: true,
-    getTarget: mask,
+    target: mask,
     onResize: updateHeight,
   });
   const [valued, onChange] = useValuedState<HTMLTextAreaElement>({

diff --git a/packages/tabs/src/useTabIndicatorStyle.ts b/packages/tabs/src/useTabIndicatorStyle.ts
@@ -104,7 +104,7 @@ export default function useTabIndicatorStyle({
     [ref]
   );
   useResizeObserver({
-    getTarget: tabsRef,
+    target: tabsRef,
     onResize() {
       // whenever the tabs container element is resized, it _probably_ means that the tabs
       // will be resized or moved. this means the indicator will be in the wrong place so
@@ -117,7 +117,7 @@ export default function useTabIndicatorStyle({
   // updates the width of the tab (dev utils) or if the width was not changed due to the tabs
   // container element resizing (iffy)
   useResizeObserver({
-    getTarget: () => getActiveTab(itemRefs, activeIndex),
+    target: () => getActiveTab(itemRefs, activeIndex),
     onResize() {
       updateCSSVars(itemRefs, activeIndex);
     },

diff --git a/packages/utils/src/layout/GridList.tsx b/packages/utils/src/layout/GridList.tsx
@@ -206,7 +206,7 @@ const GridList: FC<GridListProps & WithRef> = providedProps => {
     disableHeight: disableHeightObserver,
     disableWidth: disableWidthObserver,
     onResize: recalculate,
-    getTarget: ref,
+    target: ref,
   });
   const mergedStyle: CSSProperties = {
     ...style,

diff --git a/packages/utils/src/sizing/ResizeObserver.tsx b/packages/utils/src/sizing/ResizeObserver.tsx
@@ -2,7 +2,7 @@ import React, { FC, ElementType, useCallback, useState } from "react";
 
 import useResizeObserver, {
   ResizeObserverChangeEventHandler,
-  FindResizeTarget,
+  ResizeObserverTarget,
 } from "./useResizeObserver";
 
 export interface ResizeObserverProps {
@@ -42,7 +42,7 @@ export interface ResizeObserverProps {
    * Setting this to `null` will result in a "lazy Observer". The observer will not start until it has
    * been updated to be a string or an HTMLElement.
    */
-  target?: FindResizeTarget;
+  target?: ResizeObserverTarget;
 
   /**
    * The resize event handler for the resize observer. The callback will include the next height, width,
@@ -72,8 +72,12 @@ const ResizeObserver: FC<ResizeObserverProps> = providedProps => {
   } = providedProps as WithDefaultProps;
 
   const [element, setElement] = useState<HTMLElement | null>(null);
-  const getTarget = target || element;
-  useResizeObserver({ disableHeight, disableWidth, getTarget, onResize });
+  useResizeObserver({
+    disableHeight,
+    disableWidth,
+    target: target || element,
+    onResize,
+  });
   const ref = useCallback((instance: HTMLElement | null) => {
     if (!instance) {
       setElement(null);

diff --git a/packages/utils/src/sizing/useResizeObserver.ts b/packages/utils/src/sizing/useResizeObserver.ts
@@ -32,24 +32,29 @@ type TargetRef<
 > = MutableRefObject<E | null>;
 
 /**
- * A resize observer target finder. This can either be a `document.querySeletor` string,
- * an `HTMLElement`, a function that returns an `HTMLElement`, or `null`.
+ * The target element for the resize obsever. This can be one of:
+ * - null
+ * - HTMLElement
+ * - a document.querySelector string
+ * - a ref with { current: null | HTMLElement }
+ * - a function that returns
+ *   - null
+ *   - HTMLElement
  *
- * Setting this to `null` will result in a "lazy Observer". The observer will not start until it has
- * been updated to be a string or an HTMLElement.
+ * Whenever the target is resolved as `null`, the observer will be disabled.
  */
-export type FindResizeTarget<E extends HTMLElement = HTMLElement> =
-  | TargetRef<E>
-  | string
+export type ResizeObserverTarget<E extends HTMLElement = HTMLElement> =
+  | null
   | HTMLElement
-  | GetTarget
-  | null;
+  | string
+  | TargetRef<E>
+  | GetTarget;
 
 /**
  * @private
  */
 const isRefTarget = (
-  target: FindResizeTarget
+  target: ResizeObserverTarget
 ): target is MutableRefObject<HTMLElement | null> =>
   !!target &&
   typeof (target as MutableRefObject<HTMLElement | null>).current !==
@@ -58,7 +63,7 @@ const isRefTarget = (
 /**
  * @private
  */
-const isFunctionTarget = (target: FindResizeTarget): target is GetTarget =>
+const isFunctionTarget = (target: ResizeObserverTarget): target is GetTarget =>
   typeof target === "function";
 
 /**
@@ -67,7 +72,7 @@ const isFunctionTarget = (target: FindResizeTarget): target is GetTarget =>
  * @private
  */
 export function getResizeObserverTarget(
-  target: FindResizeTarget
+  target: ResizeObserverTarget
 ): HTMLElement | null {
   if (isRefTarget(target)) {
     return target.current;
@@ -133,62 +138,7 @@ export interface ResizeObserverOptions<E extends HTMLElement = HTMLElement> {
   disableHeight?: boolean;
   disableWidth?: boolean;
   onResize: ResizeObserverChangeEventHandler;
-  getTarget: FindResizeTarget<E>;
-}
-
-interface DelegatedHandler {
-  onResize: MutableRefObject<ResizeObserverChangeEventHandler>;
-  disableHeight: boolean;
-  disableWidth: boolean;
-}
-
-/**
- * This will be a "lazy-initialized" observer that every `useResizeObserver` (and `ResizeObserver`)
- * hook into since it is more performant than creating multiple resize observer instances (see
- * link below). The way that this will work is that each element target will be put into a weakmap
- * containing a list of all resize handlers that should be run.
- *
- * Since the main "feature" of this hook is to be able to opt out of one of the sizing changes, there
- * will also be another weakmap of the previous size. Each of the target
- *
- * @see https://github.com/WICG/ResizeObserver/issues/59
- */
-let observer: ResizeObserver;
-const elementHandlersMap = new WeakMap<HTMLElement, DelegatedHandler[]>();
-const previousSizeMap = new WeakMap<HTMLElement, ElementSize>();
-
-/**
- * This is the shared entry measurer that will call all the resize handlers
- * for each target. It will also check if there actually was a size change based
- * on the configuration provided before calling the resize callback.
- */
-function measure(entries: ResizeObserverEntry[]): void {
-  for (let i = 0; i < entries.length; i += 1) {
-    const entry = entries[i];
-    const target = entry.target as HTMLElement;
-    const handlers = elementHandlersMap.get(target) || [];
-    const { height, width } = entry.contentRect;
-    const { scrollHeight, scrollWidth } = target;
-    const nextSize: ElementSize = {
-      height,
-      width,
-      scrollHeight,
-      scrollWidth,
-    };
-
-    const prevSize = previousSizeMap.get(target);
-    const isNewHeight = isHeightChange(prevSize, nextSize);
-    const isNewWidth = isWidthChange(prevSize, nextSize);
-    handlers.forEach(({ onResize, disableHeight, disableWidth }) => {
-      if ((isNewHeight && !disableHeight) || (isNewWidth && !disableWidth)) {
-        onResize.current({
-          ...nextSize,
-          element: target,
-        });
-      }
-    });
-    previousSizeMap.set(target, nextSize);
-  }
+  target: ResizeObserverTarget<E>;
 }
 
 /**
@@ -201,69 +151,46 @@ export default function useResizeObserver<E extends HTMLElement>({
   disableHeight = false,
   disableWidth = false,
   onResize,
-  getTarget,
+  target,
 }: ResizeObserverOptions<E>): void {
-  // have to put it into an "instance variable ref" since you'll generally be
-  // doing this with arrow functions and it's pretty bad for performance with
-  // the measurer to observer and unobserve each render especially when the
-  // callback updates the state
-  const resizeHandler = useRef(onResize);
   useEffect(() => {
-    resizeHandler.current = onResize;
-  });
-
-  const [target, setTarget] = useState(() =>
-    getResizeObserverTarget(getTarget)
-  );
-  useEffect(() => {
-    const initialTarget = getResizeObserverTarget(getTarget);
-    if (initialTarget !== target) {
-      setTarget(initialTarget);
-    }
-
-    // want componentDidMount only for this since there are some resize targets that
-    // use refs which aren't available until this phase.
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, []);
-  const prevGetTarget = useRef(getTarget);
-  if (prevGetTarget.current !== getTarget) {
-    prevGetTarget.current = getTarget;
-    const nextTarget = getResizeObserverTarget(getTarget);
-    if (nextTarget !== target) {
-      setTarget(nextTarget);
-    }
-  }
-
-  useEffect(() => {
-    if (!target || (disableHeight && disableWidth)) {
+    if (disableHeight && disableWidth) {
       return;
     }
 
-    if (!observer) {
-      observer = new ResizeObserverPolyfill(measure);
+    const resizeTarget = getResizeObserverTarget(target);
+    if (!resizeTarget) {
+      return;
     }
 
-    observer.observe(target);
-    const handlers = elementHandlersMap.get(target) || [];
-    const handler = { onResize: resizeHandler, disableHeight, disableWidth };
-    handlers.push(handler);
-    elementHandlersMap.set(target, handlers);
-    return () => {
-      observer.unobserve(target);
-      const currentHandlers = elementHandlersMap.get(target) || [];
-      if (currentHandlers.length === 1) {
-        elementHandlersMap.delete(target);
-        return;
+    let prevSize: ElementSize | undefined;
+    const observer = new ResizeObserverPolyfill(entries => {
+      for (let i = 0; i < entries.length; i += 1) {
+        const entry = entries[i];
+        const target = entry.target as HTMLElement;
+        const { height, width } = entry.contentRect;
+        const { scrollHeight, scrollWidth } = target;
+        const nextSize: ElementSize = {
+          height,
+          width,
+          scrollHeight,
+          scrollWidth,
+        };
+
+        const isNewHeight = isHeightChange(prevSize, nextSize);
+        const isNewWidth = isWidthChange(prevSize, nextSize);
+        prevSize = nextSize;
+        if ((isNewHeight && !disableHeight) || (isNewWidth && !disableWidth)) {
+          onResize({
+            ...nextSize,
+            element: target,
+          });
+        }
       }
+    });
 
-      const i = currentHandlers.findIndex(h => h === handler);
-      if (i !== -1) {
-        currentHandlers.splice(i, 1);
-      } else {
-        throw new Error(
-          "Unable to clean up a ResizeObserver handler since it did not exist at the time of clean up."
-        );
-      }
+    return () => {
+      observer.disconnect();
     };
-  }, [disableHeight, disableWidth, target]);
+  }, [target, onResize, disableHeight, disableWidth]);
 }