[core] fix(Popover): React 18 strict mode compat (#6458)

packages/core/src/common/refs.ts

@@ -30,7 +30,7 @@ export function isRefCallback<T>(value: React.Ref<T> | undefined): value is Reac
 export function setRef<T>(refTarget: React.Ref<T> | undefined, ref: T | null): void {
     if (isRefObject<T>(refTarget)) {
         // HACKHACK: .current property is readonly
-        (refTarget.current as any) = ref;
+        (refTarget.current as T | null) = ref;
     } else if (isRefCallback(refTarget)) {
         refTarget(ref);
     }

packages/core/src/components/popover/popover.tsx

@@ -19,7 +19,15 @@ import classNames from "classnames";
 import * as React from "react";
 import { Manager, Modifier, Popper, PopperChildrenProps, Reference, ReferenceChildrenProps } from "react-popper";
 
-import { AbstractPureComponent, Classes, DISPLAYNAME_PREFIX, HTMLDivProps, refHandler, Utils } from "../../common";
+import {
+    AbstractPureComponent,
+    Classes,
+    DISPLAYNAME_PREFIX,
+    HTMLDivProps,
+    mergeRefs,
+    refHandler,
+    Utils,
+} from "../../common";
 import * as Errors from "../../common/errors";
 import { Overlay } from "../overlay/overlay";
 import { ResizeSensor } from "../resize-sensor/resizeSensor";
@@ -172,11 +180,14 @@ export class Popover<
     public popoverElement: HTMLElement | null = null;
 
     /** Popover ref handler */
-    private popoverRef: React.Ref<HTMLDivElement> = refHandler(this, "popoverElement", this.props.popoverRef);
+    private popoverRef: React.RefCallback<HTMLDivElement> = refHandler(this, "popoverElement", this.props.popoverRef);
 
     /**
      * Target DOM element ref.
      *
+     * N.B. this must be a ref object since we pass it to `<ResizeSensor>`, which needs to know about the target
+     * DOM element in order to observe its dimensions.
+     *
      * @public for testing
      */
     public targetRef = React.createRef<HTMLElement>();
@@ -236,9 +247,11 @@ export class Popover<
             return this.renderTarget({ ref: noop });
         }
 
+        // Important: do not use <Reference innerRef> since it has a bug when used in React 18 strict mode
+        // see https://github.com/floating-ui/react-popper/pull/459
         return (
             <Manager>
-                <Reference innerRef={this.targetRef}>{this.renderTarget}</Reference>
+                <Reference>{this.renderTarget}</Reference>
                 <Popper
                     innerRef={this.popoverRef}
                     placement={placement ?? positionToPlacement(position)}
@@ -314,7 +327,7 @@ export class Popover<
      */
     public reposition = () => this.popperScheduleUpdate?.();
 
-    private renderTarget = ({ ref }: ReferenceChildrenProps) => {
+    private renderTarget = ({ ref: popperChildRef }: ReferenceChildrenProps) => {
         const { children, className, fill, openOnTargetFocus, renderTarget } = this.props;
         const { isOpen } = this.state;
         const isControlled = this.isControlled();
@@ -325,6 +338,10 @@ export class Popover<
             targetTagName = "div";
         }
 
+        // react-popper has a wide type for this ref, but we can narrow it based on the source
+        // see https://github.com/floating-ui/react-popper/blob/beac280d61082852c4efc302be902911ce2d424c/src/Reference.js#L17
+        const ref = mergeRefs(popperChildRef as React.RefCallback<HTMLElement>, this.targetRef);
+
         const targetEventHandlers: PopoverHoverTargetHandlers<T> | PopoverClickTargetHandlers<T> =
             isHoverInteractionKind
                 ? {
@@ -404,8 +421,8 @@ export class Popover<
             target = wrappedTarget;
         }
 
-        // N.B. we must attach the ref ('wrapped' with react-popper functionality) to the DOM element here and
-        // let ResizeSensor know about it
+        // No need to use the merged `ref` here, that only needs to be forwarded to the child node so that React can
+        // notify both popper.js and our components about the mounted DOM element.
         return (
             <ResizeSensor targetRef={this.targetRef} onResize={this.reposition}>
                 {target}
@@ -729,8 +746,7 @@ export class Popover<
 
     private updateDarkParent() {
         if (this.props.usePortal && this.state.isOpen) {
-            const hasDarkParent =
-                this.targetRef.current != null && this.targetRef.current.closest(`.${Classes.DARK}`) != null;
+            const hasDarkParent = this.targetRef.current?.closest(`.${Classes.DARK}`) != null;
             this.setState({ hasDarkParent });
         }
     }

packages/core/src/components/resize-sensor/resizeSensor.tsx

@@ -74,13 +74,14 @@ export class ResizeSensor extends AbstractPureComponent<ResizeSensorProps> {
 
     private prevElement: HTMLElement | undefined = undefined;
 
-    private observer =
-        globalThis.ResizeObserver != null ? new ResizeObserver(entries => this.props.onResize?.(entries)) : undefined;
+    private observer: ResizeObserver | undefined;
 
     public render(): React.ReactNode {
         const onlyChild = React.Children.only(this.props.children);
 
-        // if we're provided a ref to the child already, we don't need to attach one ourselves
+        // If we're provided a mutable ref to the child element already, we must re-use that one. This is necessary
+        // in cases where the child node is not a native DOM element and does not use `React.forwardRef`, since
+        // there's no way for us to know how to attach to the underlying DOM node.
         if (this.props.targetRef !== undefined) {
             return onlyChild;
         }
@@ -89,6 +90,7 @@ export class ResizeSensor extends AbstractPureComponent<ResizeSensorProps> {
     }
 
     public componentDidMount() {
+        this.observer = new ResizeObserver(entries => this.props.onResize?.(entries));
         this.observeElement();
     }
 
@@ -107,7 +109,7 @@ export class ResizeSensor extends AbstractPureComponent<ResizeSensorProps> {
      * re-observe.
      */
     private observeElement(force = false) {
-        if (this.observer == null) {
+        if (this.observer === undefined) {
             return;
         }
 

packages/core/test/resize-sensor/resizeSensorTests.tsx

@@ -32,11 +32,12 @@ describe("<ResizeSensor>", () => {
         wrapper?.unmount();
         wrapper?.detach();
     });
+
     after(() => testsContainerElement.remove());
 
     it("onResize is called when size changes", async () => {
         const onResize = spy();
-        mountResizeSensor(onResize);
+        mountResizeSensor({ onResize });
         await resize({ width: 200 });
         await resize({ height: 100 });
         await resize({ width: 55 });
@@ -46,7 +47,7 @@ describe("<ResizeSensor>", () => {
 
     it("onResize is NOT called redundantly when size is unchanged", async () => {
         const onResize = spy();
-        mountResizeSensor(onResize);
+        mountResizeSensor({ onResize });
         await resize({ width: 200 });
         await resize({ width: 200 }); // this one should be ignored
         assert.equal(onResize.callCount, 1);
@@ -55,7 +56,7 @@ describe("<ResizeSensor>", () => {
 
     it("onResize is called when element changes", async () => {
         const onResize = spy();
-        mountResizeSensor(onResize);
+        mountResizeSensor({ onResize });
         await resize({ width: 200, id: 1 });
         await resize({ width: 200, id: 2 }); // not ignored bc element recreated
         await resize({ width: 55, id: 3 });
@@ -64,7 +65,7 @@ describe("<ResizeSensor>", () => {
 
     it("onResize can be changed", async () => {
         const onResize1 = spy();
-        mountResizeSensor(onResize1);
+        mountResizeSensor({ onResize: onResize1 });
         await resize({ width: 200, id: 1 });
 
         const onResize2 = spy();
@@ -76,16 +77,29 @@ describe("<ResizeSensor>", () => {
         assert.equal(onResize2.callCount, 2, "second callback should have been called exactly twice");
     });
 
-    function mountResizeSensor(onResize: ResizeSensorProps["onResize"]) {
+    it("still works when user sets their own targetRef", async () => {
+        const onResize = spy();
+        const targetRef = React.createRef<HTMLElement>();
+        const RESIZE_WIDTH = 200;
+        mountResizeSensor({ onResize, targetRef });
+        await resize({ width: RESIZE_WIDTH });
+        assert.equal(onResize.callCount, 1, "onResize should be called");
+        assertResizeArgs(onResize, [`${RESIZE_WIDTH}x0`]);
+        assert.isNotNull(targetRef.current, "user-provided targetRef should be set");
+        assert.strictEqual(targetRef.current?.clientWidth, RESIZE_WIDTH, "user-provided targetRef.current.clientWidth");
+    });
+
+    function mountResizeSensor(props: Omit<ResizeSensorProps, "children">) {
         return (wrapper = mount<ResizeTesterProps>(
-            <ResizeTester id={0} onResize={onResize} />,
+            <ResizeTester id={0} {...props} />,
             // must be in the DOM for measurement
             { attachTo: testsContainerElement },
         ));
     }
 
     function resize(size: SizeProps) {
         wrapper!.setProps(size);
+        wrapper!.update();
         return new Promise(resolve => setTimeout(resolve, 30));
     }
 
@@ -109,6 +123,6 @@ interface SizeProps {
 type ResizeTesterProps = Omit<ResizeSensorProps, "children"> & SizeProps;
 const ResizeTester: React.FC<ResizeTesterProps> = ({ id, width, height, ...sensorProps }) => (
     <ResizeSensor {...sensorProps}>
-        <div key={id} style={{ width, height }} />
+        <div key={id} style={{ width, height }} ref={sensorProps.targetRef as React.RefObject<HTMLDivElement>} />
     </ResizeSensor>
 );

packages/select/test/suggestTests.tsx

@@ -63,6 +63,7 @@ describe("Suggest", () => {
                 inputValueRenderer={inputValueRenderer}
                 popoverProps={{ isOpen: true, usePortal: false }}
             />,
+            { attachTo: testsContainerElement },
         ),
     );
 
@@ -337,7 +338,9 @@ describe("Suggest", () => {
     });
 
     function suggest(props: Partial<SuggestProps<Film>> = {}) {
-        return mount<Suggest<Film>>(<Suggest<Film> {...defaultProps} {...handlers} {...props} />);
+        return mount<Suggest<Film>>(<Suggest<Film> {...defaultProps} {...handlers} {...props} />, {
+            attachTo: testsContainerElement,
+        });
     }
 });
 

packages/table/src/deprecatedAliases.ts

@@ -32,12 +32,12 @@ export {
     /** @deprecated import JSONFormat instead */
     JSONFormat as JSONFormat2,
     /** @deprecated import JSONFormatProps instead */
-    JSONFormatProps as JSONFormat2Props,
+    type JSONFormatProps as JSONFormat2Props,
 } from "./cell/formats/jsonFormat";
 
 export {
     /** @deprecated import TruncatedFormat instead */
     TruncatedFormat as TruncatedFormat2,
     /** @deprecated import TruncatedFormatProps instead */
-    TruncatedFormatProps as TruncatedFormat2Props,
+    type TruncatedFormatProps as TruncatedFormat2Props,
 } from "./cell/formats/truncatedFormat";