From dba16239560bb46d717e97034a5e5570bb474502 Mon Sep 17 00:00:00 2001 From: Adi Dahiya Date: Thu, 12 Oct 2023 14:15:11 -0400 Subject: [PATCH] [core] fix(Popover): React 18 strict mode compat (#6458) --- packages/core/src/common/refs.ts | 2 +- .../core/src/components/popover/popover.tsx | 32 ++++++++++++++----- .../components/resize-sensor/resizeSensor.tsx | 10 +++--- .../test/resize-sensor/resizeSensorTests.tsx | 28 ++++++++++++---- packages/select/test/suggestTests.tsx | 5 ++- packages/table/src/deprecatedAliases.ts | 4 +-- 6 files changed, 58 insertions(+), 23 deletions(-) diff --git a/packages/core/src/common/refs.ts b/packages/core/src/common/refs.ts index 88ba9c460a..2b14b6028b 100644 --- a/packages/core/src/common/refs.ts +++ b/packages/core/src/common/refs.ts @@ -30,7 +30,7 @@ export function isRefCallback(value: React.Ref | undefined): value is Reac export function setRef(refTarget: React.Ref | undefined, ref: T | null): void { if (isRefObject(refTarget)) { // HACKHACK: .current property is readonly - (refTarget.current as any) = ref; + (refTarget.current as T | null) = ref; } else if (isRefCallback(refTarget)) { refTarget(ref); } diff --git a/packages/core/src/components/popover/popover.tsx b/packages/core/src/components/popover/popover.tsx index bb15723242..9d949085ce 100644 --- a/packages/core/src/components/popover/popover.tsx +++ b/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 = refHandler(this, "popoverElement", this.props.popoverRef); + private popoverRef: React.RefCallback = refHandler(this, "popoverElement", this.props.popoverRef); /** * Target DOM element ref. * + * N.B. this must be a ref object since we pass it to ``, which needs to know about the target + * DOM element in order to observe its dimensions. + * * @public for testing */ public targetRef = React.createRef(); @@ -236,9 +247,11 @@ export class Popover< return this.renderTarget({ ref: noop }); } + // Important: do not use since it has a bug when used in React 18 strict mode + // see https://github.com/floating-ui/react-popper/pull/459 return ( - {this.renderTarget} + {this.renderTarget} 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, this.targetRef); + const targetEventHandlers: PopoverHoverTargetHandlers | PopoverClickTargetHandlers = 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 ( {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 }); } } diff --git a/packages/core/src/components/resize-sensor/resizeSensor.tsx b/packages/core/src/components/resize-sensor/resizeSensor.tsx index 01b6d1d3f6..b63d8d753b 100644 --- a/packages/core/src/components/resize-sensor/resizeSensor.tsx +++ b/packages/core/src/components/resize-sensor/resizeSensor.tsx @@ -74,13 +74,14 @@ export class ResizeSensor extends AbstractPureComponent { 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 { } public componentDidMount() { + this.observer = new ResizeObserver(entries => this.props.onResize?.(entries)); this.observeElement(); } @@ -107,7 +109,7 @@ export class ResizeSensor extends AbstractPureComponent { * re-observe. */ private observeElement(force = false) { - if (this.observer == null) { + if (this.observer === undefined) { return; } diff --git a/packages/core/test/resize-sensor/resizeSensorTests.tsx b/packages/core/test/resize-sensor/resizeSensorTests.tsx index 444601d37c..5eb5101c13 100644 --- a/packages/core/test/resize-sensor/resizeSensorTests.tsx +++ b/packages/core/test/resize-sensor/resizeSensorTests.tsx @@ -32,11 +32,12 @@ describe("", () => { 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("", () => { 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("", () => { 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("", () => { it("onResize can be changed", async () => { const onResize1 = spy(); - mountResizeSensor(onResize1); + mountResizeSensor({ onResize: onResize1 }); await resize({ width: 200, id: 1 }); const onResize2 = spy(); @@ -76,9 +77,21 @@ describe("", () => { 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(); + 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) { return (wrapper = mount( - , + , // must be in the DOM for measurement { attachTo: testsContainerElement }, )); @@ -86,6 +99,7 @@ describe("", () => { 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 & SizeProps; const ResizeTester: React.FC = ({ id, width, height, ...sensorProps }) => ( -
+
} /> ); diff --git a/packages/select/test/suggestTests.tsx b/packages/select/test/suggestTests.tsx index 2791ed2ab7..100e14a4ba 100644 --- a/packages/select/test/suggestTests.tsx +++ b/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> = {}) { - return mount>( {...defaultProps} {...handlers} {...props} />); + return mount>( {...defaultProps} {...handlers} {...props} />, { + attachTo: testsContainerElement, + }); } }); diff --git a/packages/table/src/deprecatedAliases.ts b/packages/table/src/deprecatedAliases.ts index 9eafb3b8da..5b258c3fb3 100644 --- a/packages/table/src/deprecatedAliases.ts +++ b/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";