From 0429377442c5c654e70508bea7e0ef5f69dff65d Mon Sep 17 00:00:00 2001 From: Jordan Pittman Date: Tue, 19 Sep 2023 16:42:00 -0400 Subject: [PATCH 1/4] =?UTF-8?q?Make=20sure=20`as=3D{Fragment}`=20doesn?= =?UTF-8?q?=E2=80=99t=20result=20in=20a=20render=20loop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/components/disclosure/disclosure.tsx | 5 ++ .../@headlessui-react/src/utils/render.ts | 67 ++++++++++++++----- 2 files changed, 55 insertions(+), 17 deletions(-) diff --git a/packages/@headlessui-react/src/components/disclosure/disclosure.tsx b/packages/@headlessui-react/src/components/disclosure/disclosure.tsx index 8b5509e29c..a1945bdf59 100644 --- a/packages/@headlessui-react/src/components/disclosure/disclosure.tsx +++ b/packages/@headlessui-react/src/components/disclosure/disclosure.tsx @@ -28,6 +28,7 @@ import { Features, forwardRefWithAs, render, + useMergeRefsFn, type HasDisplayName, type PropsForFeatures, type RefProp, @@ -267,6 +268,7 @@ function ButtonFn( let internalButtonRef = useRef(null) let buttonRef = useSyncRefs(internalButtonRef, ref, !isWithinPanel ? state.buttonRef : null) + let mergeRefs = useMergeRefsFn() useEffect(() => { if (isWithinPanel) return @@ -345,6 +347,7 @@ function ButtonFn( } return render({ + mergeRefs, ourProps, theirProps, slot, @@ -374,6 +377,7 @@ function PanelFn( let { id = `headlessui-disclosure-panel-${internalId}`, ...theirProps } = props let [state, dispatch] = useDisclosureContext('Disclosure.Panel') let { close } = useDisclosureAPIContext('Disclosure.Panel') + let mergeRefs = useMergeRefsFn() let panelRef = useSyncRefs(ref, state.panelRef, (el) => { startTransition(() => dispatch({ type: el ? ActionTypes.LinkPanel : ActionTypes.UnlinkPanel })) @@ -408,6 +412,7 @@ function PanelFn( return ( {render({ + mergeRefs, ourProps, theirProps, slot, diff --git a/packages/@headlessui-react/src/utils/render.ts b/packages/@headlessui-react/src/utils/render.ts index c2af718164..16529354ce 100644 --- a/packages/@headlessui-react/src/utils/render.ts +++ b/packages/@headlessui-react/src/utils/render.ts @@ -4,6 +4,9 @@ import { forwardRef, Fragment, isValidElement, + MutableRefObject, + useCallback, + useRef, type ElementType, type ReactElement, type Ref, @@ -54,6 +57,7 @@ export function render & PropsForFeatures> & { ref?: Ref @@ -64,11 +68,14 @@ export function render }) { + mergeRefs = mergeRefs ?? defaultMergeRefs + let props = mergeProps(theirProps, ourProps) // Visible always render - if (visible) return _render(props, slot, defaultTag, name) + if (visible) return _render(props, slot, defaultTag, name, mergeRefs) let featureFlags = features ?? Features.None @@ -76,7 +83,7 @@ export function render // When the `static` prop is passed as `true`, then the user is in control, thus we don't care about anything else - if (isStatic) return _render(rest, slot, defaultTag, name) + if (isStatic) return _render(rest, slot, defaultTag, name, mergeRefs) } if (featureFlags & Features.RenderStrategy) { @@ -92,21 +99,23 @@ export function render( props: Props & { ref?: unknown }, slot: TSlot = {} as TSlot, tag: ElementType, - name: string + name: string, + mergeRefs: ReturnType ) { let { as: Component = tag, @@ -189,7 +198,7 @@ function _render( mergeProps(resolvedChildren.props as any, compact(omit(rest, ['ref']))), dataAttributes, refRelatedProps, - mergeRefs((resolvedChildren as any).ref, refRelatedProps.ref), + { ref: mergeRefs((resolvedChildren as any).ref, refRelatedProps.ref) }, classNameProps ) ) @@ -208,20 +217,44 @@ function _render( ) } -function mergeRefs(...refs: any[]) { - return { - ref: refs.every((ref) => ref == null) - ? undefined - : (value: any) => { - for (let ref of refs) { - if (ref == null) continue - if (typeof ref === 'function') ref(value) - else ref.current = value - } - }, +export function useMergeRefsFn() { + type MaybeRef = MutableRefObject | ((value: T) => void) | null | undefined + let currentRefs = useRef[]>([]) + let mergedRef = useCallback((value) => { + for (let ref of currentRefs.current) { + if (ref == null) continue + if (typeof ref === 'function') ref(value) + else ref.current = value + } + }, []) + + return (...refs: any[]) => { + if (refs.every((ref) => ref == null)) { + return undefined + } + + currentRefs.current = refs + return mergedRef } } +// This does not produce a stable function to use as a ref +// But we only use it in the case of as={Fragment} +// And it should really only re-render if setting the ref causes the parent to re-render unconditionally +// which then causes the child to re-render resulting in a render loop +// TODO: Add tests for this somehow +function defaultMergeRefs(...refs: any[]) { + return refs.every((ref) => ref == null) + ? undefined + : (value: any) => { + for (let ref of refs) { + if (ref == null) continue + if (typeof ref === 'function') ref(value) + else ref.current = value + } + } +} + function mergeProps(...listOfProps: Props[]) { if (listOfProps.length === 0) return {} if (listOfProps.length === 1) return listOfProps[0] From 09dfe7c55c74f331e2817dbad8c426dbc4e151a7 Mon Sep 17 00:00:00 2001 From: Jordan Pittman Date: Tue, 19 Sep 2023 16:50:29 -0400 Subject: [PATCH 2/4] Add comment about usage --- packages/@headlessui-react/src/utils/render.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/packages/@headlessui-react/src/utils/render.ts b/packages/@headlessui-react/src/utils/render.ts index 16529354ce..b683ca830c 100644 --- a/packages/@headlessui-react/src/utils/render.ts +++ b/packages/@headlessui-react/src/utils/render.ts @@ -217,6 +217,19 @@ function _render( ) } +/** + * This is a singleton hook. **You can ONLY call the returned + * function *once* to produce expected results.** If you need + * to call `mergeRefs()` multiple times you need to create a + * separate function for each invocation. This happens as we + * store the list of `refs` to update and always return the + * same function that refers to that list of refs. + * + * You shouldn't normally read refs during render but this + * should actually be okay because React itself is calling + * the `function` that updates these refs and can only do + * so once the ref that contains the list is updated. + */ export function useMergeRefsFn() { type MaybeRef = MutableRefObject | ((value: T) => void) | null | undefined let currentRefs = useRef[]>([]) From 0566b545801f3b61adc6767a4d59acd7d502c968 Mon Sep 17 00:00:00 2001 From: Jordan Pittman Date: Tue, 19 Sep 2023 16:56:58 -0400 Subject: [PATCH 3/4] Fix render loop on popover panel too --- packages/@headlessui-react/src/components/popover/popover.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/@headlessui-react/src/components/popover/popover.tsx b/packages/@headlessui-react/src/components/popover/popover.tsx index 8e991f3858..98f1cc1221 100644 --- a/packages/@headlessui-react/src/components/popover/popover.tsx +++ b/packages/@headlessui-react/src/components/popover/popover.tsx @@ -49,6 +49,7 @@ import { Features, forwardRefWithAs, render, + useMergeRefsFn, type HasDisplayName, type PropsForFeatures, type RefProp, @@ -755,6 +756,7 @@ function PanelFn( dispatch({ type: ActionTypes.SetPanel, panel }) }) let ownerDocument = useOwnerDocument(internalPanelRef) + let mergeRefs = useMergeRefsFn() useIsoMorphicEffect(() => { dispatch({ type: ActionTypes.SetPanelId, panelId: id }) @@ -934,6 +936,7 @@ function PanelFn( /> )} {render({ + mergeRefs, ourProps, theirProps, slot, From 8eda90c642fc69721ab4441eb24be18701192279 Mon Sep 17 00:00:00 2001 From: Jordan Pittman Date: Tue, 19 Sep 2023 16:58:14 -0400 Subject: [PATCH 4/4] Update changelog --- packages/@headlessui-react/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/@headlessui-react/CHANGELOG.md b/packages/@headlessui-react/CHANGELOG.md index b519b5a170..de0f4915e0 100644 --- a/packages/@headlessui-react/CHANGELOG.md +++ b/packages/@headlessui-react/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Ensure blurring the `Combobox.Input` component closes the `Combobox` ([#2712](https://github.com/tailwindlabs/headlessui/pull/2712)) - Allow changes to the `className` prop when the `` component is currently not transitioning ([#2722](https://github.com/tailwindlabs/headlessui/pull/2722)) - Export (internal-only) component interfaces for TypeScript compiler ([#2313](https://github.com/tailwindlabs/headlessui/pull/2313)) +- Fix infinite render-loop for `` and `` when `as={Fragment}` ([#2760](https://github.com/tailwindlabs/headlessui/pull/2760)) ### Added