From 5c60d6a11512086f395ace352eec868fcd748f44 Mon Sep 17 00:00:00 2001 From: Chris Villa Date: Sat, 18 Jan 2025 08:58:07 +0000 Subject: [PATCH] fix: improve stability of resolveFields API --- .../Puck/components/Fields/index.tsx | 146 +----- .../__tests__/use-resolved-fields.spec.tsx | 451 ++++++++++++++++++ packages/core/lib/selector-is.ts | 4 + packages/core/lib/use-on-value-change.ts | 19 + packages/core/lib/use-parent.ts | 34 +- packages/core/lib/use-resolved-fields.ts | 157 ++++++ 6 files changed, 662 insertions(+), 149 deletions(-) create mode 100644 packages/core/lib/__tests__/use-resolved-fields.spec.tsx create mode 100644 packages/core/lib/selector-is.ts create mode 100644 packages/core/lib/use-on-value-change.ts create mode 100644 packages/core/lib/use-resolved-fields.ts diff --git a/packages/core/components/Puck/components/Fields/index.tsx b/packages/core/components/Puck/components/Fields/index.tsx index 526488b0b1..4cf4553bfa 100644 --- a/packages/core/components/Puck/components/Fields/index.tsx +++ b/packages/core/components/Puck/components/Fields/index.tsx @@ -6,24 +6,18 @@ import { replaceAction, setAction, } from "../../../../reducer"; -import { ComponentData, RootData, UiState } from "../../../../types"; -import type { Field, Fields as FieldsType } from "../../../../types"; +import { UiState } from "../../../../types"; import { AutoFieldPrivate } from "../../../AutoField"; import { useAppContext } from "../../context"; import styles from "./styles.module.css"; import { getClassNameFactory } from "../../../../lib"; -import { ReactNode, useCallback, useEffect, useMemo, useState } from "react"; +import { ReactNode, useMemo } from "react"; import { ItemSelector } from "../../../../lib/get-item"; -import { getChanged } from "../../../../lib/get-changed"; -import { useParent } from "../../../../lib/use-parent"; +import { useResolvedFields } from "../../../../lib/use-resolved-fields"; const getClassName = getClassNameFactory("PuckFields", styles); -const defaultPageFields: Record = { - title: { type: "text" }, -}; - const DefaultFields = ({ children, }: { @@ -34,139 +28,7 @@ const DefaultFields = ({ return <>{children}; }; -type ComponentOrRootData = Omit, "type">; - -const useResolvedFields = (): [FieldsType, boolean] => { - const { selectedItem, state, config } = useAppContext(); - const parent = useParent(); - - const { data } = state; - - const rootFields = config.root?.fields || defaultPageFields; - - const componentConfig = selectedItem - ? config.components[selectedItem.type] - : null; - - const defaultFields = useMemo( - () => - (selectedItem - ? (componentConfig?.fields as Record>) - : rootFields) || {}, - [selectedItem, rootFields, componentConfig?.fields] - ); - - // DEPRECATED - const rootProps = data.root.props || data.root; - - const [lastSelectedData, setLastSelectedData] = useState< - Partial - >({}); - const [resolvedFields, setResolvedFields] = useState(defaultFields); - const [fieldsLoading, setFieldsLoading] = useState(false); - - const defaultResolveFields = ( - _componentData: ComponentOrRootData, - _params: { - fields: FieldsType; - lastData: Partial | null; - lastFields: FieldsType; - changed: Record; - } - ) => defaultFields; - - const componentData: ComponentOrRootData = selectedItem - ? selectedItem - : { props: rootProps, readOnly: data.root.readOnly }; - - const hasComponentResolver = selectedItem && componentConfig?.resolveFields; - const hasRootResolver = !selectedItem && config.root?.resolveFields; - const hasResolver = hasComponentResolver || hasRootResolver; - - const resolveFields = useCallback( - async (fields: FieldsType = {}) => { - const lastData = - lastSelectedData.props?.id === componentData.props.id - ? lastSelectedData - : null; - - const changed = getChanged(componentData, lastData); - - setLastSelectedData(componentData); - - if (hasComponentResolver) { - return await componentConfig!.resolveFields!( - componentData as ComponentData, - { - changed, - fields, - lastFields: resolvedFields, - lastData: lastData as ComponentData, - appState: state, - parent, - } - ); - } - - if (hasRootResolver) { - return await config.root!.resolveFields!(componentData, { - changed, - fields, - lastFields: resolvedFields, - lastData: lastData as RootData, - appState: state, - parent, - }); - } - - return defaultResolveFields(componentData, { - changed, - fields, - lastFields: resolvedFields, - lastData, - }); - }, - [data, config, componentData, selectedItem, resolvedFields, state, parent] - ); - - const [hasParent, setHasParent] = useState(false); - - useEffect(() => { - setHasParent(!!parent); - }, [parent]); - - useEffect(() => { - // Must either be in default zone, or have parent - if ( - !state.ui.itemSelector?.zone || - state.ui.itemSelector?.zone === "default-zone" || - hasParent - ) { - if (hasResolver) { - setFieldsLoading(true); - - resolveFields(defaultFields).then((fields) => { - setResolvedFields(fields || {}); - - setFieldsLoading(false); - }); - } else { - setResolvedFields(defaultFields); - } - } - }, [ - data, - defaultFields, - state.ui.itemSelector, - selectedItem, - hasResolver, - hasParent, - ]); - - return [resolvedFields, fieldsLoading]; -}; - -export const Fields = () => { +export const Fields = ({ wrapFields = true }: { wrapFields?: boolean }) => { const { selectedItem, state, diff --git a/packages/core/lib/__tests__/use-resolved-fields.spec.tsx b/packages/core/lib/__tests__/use-resolved-fields.spec.tsx new file mode 100644 index 0000000000..6b860e7681 --- /dev/null +++ b/packages/core/lib/__tests__/use-resolved-fields.spec.tsx @@ -0,0 +1,451 @@ +import { renderHook, act, waitFor } from "@testing-library/react"; +import { useResolvedFields } from "../use-resolved-fields"; +import { useAppContext } from "../../components/Puck/context"; +import { useParent } from "../use-parent"; +import { AppState, ComponentData, Data, Fields, UiState } from "../../types"; + +jest.mock("../../components/Puck/context", () => ({ + useAppContext: jest.fn(), +})); + +jest.mock("../use-parent", () => ({ + useParent: jest.fn(), +})); + +const state: { data: Data; ui: Partial } = { + data: { root: { props: {} }, content: [] }, + ui: { + itemSelector: null, + }, +}; + +const blankContext = { + selectedItem: undefined as ComponentData | undefined, + state, + config: { + root: { + fields: { title: { type: "text" } }, + }, + components: { + Flex: { + fields: {}, + }, + Heading: { + fields: { title: { type: "text" } }, + }, + }, + }, +}; + +const contextWithRootResolver = (resolveFields: any) => { + return { + ...blankContext, + config: { + ...blankContext.config, + root: { + ...blankContext.config.root, + resolveFields, + }, + }, + }; +}; + +const contextWithItemResolver = (resolveFields: any) => { + return { + ...blankContext, + state: { + ...blankContext.state, + data: { + ...blankContext.state.data, + content: [ + { + type: "Heading", + props: { + id: "heading-1", + title: "Hello, world", + }, + }, + ], + }, + }, + config: { + ...blankContext.config, + components: { + Heading: { + ...blankContext.config.components.Heading, + resolveFields, + }, + }, + }, + }; +}; + +const contextWithDropZoneItemResolver = (resolveFields: any) => { + return { + ...blankContext, + state: { + ...blankContext.state, + data: { + ...blankContext.state.data, + content: [ + { + type: "Flex", + props: { + id: "flex-1", + }, + }, + ], + zones: { + // Create an imaginary zone + "flex-1:zone": [ + { + type: "Heading", + props: { + id: "heading-1", + title: "Hello, world", + }, + }, + ], + }, + }, + }, + config: { + ...blankContext.config, + components: { + Flex: {}, + Heading: { + ...blankContext.config.components.Heading, + resolveFields, + }, + }, + }, + }; +}; + +const useAppContextMock = useAppContext as jest.Mock; +const useParentMock = useParent as jest.Mock; + +type RenderHookReturn = ReturnType< + typeof renderHook<[Fields, boolean], boolean> +>; + +describe("use-resolved-fields", () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + const defaultParams: RenderHookReturn = { + result: { current: [{}, false] }, + rerender: () => {}, + unmount: () => {}, + }; + + let params: RenderHookReturn = defaultParams; + + beforeEach(() => { + params = defaultParams; + useAppContextMock.mockClear(); + useParentMock.mockClear(); + }); + + it("returns the default fields when no resolver is defined", async () => { + useAppContextMock.mockReturnValue(blankContext); + useParentMock.mockReturnValue(null); + + params = renderHook(() => useResolvedFields()); + const { result } = params; + + expect(result.current[0]).toEqual({ title: { type: "text" } }); + expect(result.current[1]).toBe(false); + }); + + describe("with resolveFields on the root", () => { + it("calls it immediately", async () => { + const mockResolveFields = jest.fn().mockResolvedValue({ + title: { type: "textarea" }, + }); + + const context = contextWithRootResolver(mockResolveFields); + + useAppContextMock.mockReturnValue(context); + useParentMock.mockReturnValue(null); + + await act(() => { + params = renderHook(() => useResolvedFields()); + }); + + const { result } = params; + + expect(result.current[0]).toEqual({ title: { type: "textarea" } }); + expect(result.current[1]).toBe(false); + expect(mockResolveFields).toHaveBeenCalledTimes(1); + expect(mockResolveFields).toHaveBeenCalledWith( + { + props: {}, + readOnly: undefined, + }, + { + appState: { + data: { content: [], root: { props: {} } }, + ui: { itemSelector: null }, + }, + changed: {}, + fields: { title: { type: "text" } }, + lastData: {}, + lastFields: { title: { type: "text" } }, + parent: null, + } + ); + }); + + it("defers state update when async", async () => { + const mockResolveFields = jest.fn().mockResolvedValue({ + title: { type: "textarea" }, + }); + + const context = contextWithRootResolver(async (...args: any) => { + return new Promise((resolve) => { + setTimeout(() => { + resolve(mockResolveFields(...args)); + }, 50); + }); + }); + + useAppContextMock.mockReturnValue(context); + useParentMock.mockReturnValue(null); + + await act(() => { + params = renderHook(() => useResolvedFields()); + }); + + const { result } = params; + + expect(result.current[0]).toEqual({ title: { type: "text" } }); + expect(result.current[1]).toBe(true); + + await waitFor(() => { + expect(result.current[0]).toEqual({ title: { type: "textarea" } }); + expect(result.current[1]).toBe(false); + expect(mockResolveFields).toHaveBeenCalledTimes(1); + expect(mockResolveFields).toHaveBeenCalledWith( + { + props: {}, + readOnly: undefined, + }, + { + appState: { + data: { content: [], root: { props: {} } }, + ui: { itemSelector: null }, + }, + changed: {}, + fields: { title: { type: "text" } }, + lastData: {}, + lastFields: { title: { type: "text" } }, + parent: null, + } + ); + }); + }); + + it("tracks lastFields when rerendering", async () => { + const mockResolveFields = jest.fn().mockResolvedValue({ + title: { type: "textarea" }, + }); + + const context = contextWithRootResolver(mockResolveFields); + + useAppContextMock.mockReturnValue(context); + useParentMock.mockReturnValue(null); + + await act(() => { + params = renderHook(() => useResolvedFields()); + }); + + useAppContextMock.mockReturnValue({ + ...context, + state: { ...context.state, data: { root: { foo: "bar" } } }, // trigger re-render + }); + + await act(() => { + params.rerender(); + }); + + const { result } = params; + + expect(result.current[0]).toEqual({ title: { type: "textarea" } }); + expect(result.current[1]).toBe(false); + expect(mockResolveFields).toHaveBeenCalledTimes(2); + expect(mockResolveFields).toHaveBeenCalledWith( + { props: {}, readOnly: undefined }, + { + appState: { + data: { content: [], root: { props: {} } }, + ui: { itemSelector: null }, + }, + changed: {}, + fields: { title: { type: "text" } }, + lastData: {}, + lastFields: { title: { type: "textarea" } }, + parent: null, + } + ); + }); + }); + + describe("with resolveFields on a component", () => { + it("calls the resolver immediately if a relevant item is preselected", async () => { + const mockResolveFields = jest.fn().mockResolvedValue({ + title: { type: "textarea" }, + }); + + const defaultContext = contextWithItemResolver(mockResolveFields); + + const context = { + ...defaultContext, + state: { + ...defaultContext.state, + ui: { + ...defaultContext.state.ui, + itemSelector: { zone: "default-zone", index: 0 }, + }, + }, + selectedItem: defaultContext.state.data.content[0], + }; + + useAppContextMock.mockReturnValue(context); + useParentMock.mockReturnValue(null); + + await act(() => { + params = renderHook(() => useResolvedFields()); + }); + + const { result } = params; + + expect(result.current[0]).toEqual({ title: { type: "textarea" } }); + expect(result.current[1]).toBe(false); + expect(mockResolveFields).toHaveBeenCalledTimes(1); + expect(mockResolveFields).toHaveBeenCalledWith( + { props: { id: "heading-1", title: "Hello, world" }, type: "Heading" }, + { + appState: context.state, + changed: { id: true, title: true }, + fields: { title: { type: "text" } }, + lastData: null, + lastFields: { title: { type: "text" } }, + parent: null, + } + ); + }); + + it("calls the resolver after a relevant item is selected", async () => { + const mockResolveFields = jest.fn().mockResolvedValue({ + title: { type: "textarea" }, + }); + + const defaultContext = contextWithItemResolver(mockResolveFields); + + useAppContextMock.mockReturnValue(defaultContext); + useParentMock.mockReturnValue(null); + + await act(() => { + params = renderHook(() => useResolvedFields()); + }); + + const { result, rerender } = params; + + const context = { + ...defaultContext, + state: { + ...defaultContext.state, + ui: { + ...defaultContext.state.ui, + itemSelector: { zone: "default-zone", index: 0 }, + }, + }, + selectedItem: defaultContext.state.data.content[0], + }; + + useAppContextMock.mockReturnValue(context); + + await act(() => { + rerender(); + rerender(); // Deliberately trigger a 2nd rerender to ensure we don't automatically re-run the resolver + }); + + expect(result.current[0]).toEqual({ title: { type: "textarea" } }); + expect(result.current[1]).toBe(false); + expect(mockResolveFields).toHaveBeenCalledTimes(1); + expect(mockResolveFields).toHaveBeenCalledWith(expect.any(Object), { + appState: context.state, + changed: { id: true, title: true }, + fields: { title: { type: "text" } }, + lastData: null, + lastFields: { title: { type: "text" } }, + parent: null, + }); + }); + }); + + describe("with resolveFields on a component within a DropZone", () => { + it("calls the resolver as soon as the item and parent are selected", async () => { + const mockResolveFields = jest.fn().mockResolvedValue({ + title: { type: "textarea" }, + }); + + const defaultContext = contextWithDropZoneItemResolver(mockResolveFields); + + useAppContextMock.mockReturnValue(defaultContext); + useParentMock.mockReturnValue(null); + + await act(() => { + params = renderHook(() => useResolvedFields()); + }); + + expect(mockResolveFields).toHaveBeenCalledTimes(0); + + const context = { + ...defaultContext, + state: { + ...defaultContext.state, + ui: { + ...defaultContext.state.ui, + itemSelector: { zone: "flex-1:zone", index: 0 }, + }, + }, + selectedItem: defaultContext.state.data.zones?.["flex-1:zone"][0], + }; + + useAppContextMock.mockReturnValue(context); + useParentMock.mockReturnValue(context.state.data.content[0]); + + await act(() => { + params.rerender(); + }); + + // Update values and render again to make sure resolver doesn't get called more than once + useAppContextMock.mockReturnValue({ ...context }); + useParentMock.mockReturnValue({ ...context.state.data.content[0] }); + + await act(() => { + params.rerender(); + }); + + const { result } = params; + + expect(result.current[0]).toEqual({ title: { type: "textarea" } }); + expect(result.current[1]).toBe(false); + expect(mockResolveFields).toHaveBeenCalledTimes(1); + expect(mockResolveFields).toHaveBeenCalledWith( + { props: { id: "heading-1", title: "Hello, world" }, type: "Heading" }, + { + appState: context.state, + changed: { id: true, title: true }, + fields: { title: { type: "text" } }, + lastData: null, + lastFields: { title: { type: "text" } }, + parent: context.state.data.content[0], + } + ); + }); + }); +}); diff --git a/packages/core/lib/selector-is.ts b/packages/core/lib/selector-is.ts new file mode 100644 index 0000000000..1ac718a46e --- /dev/null +++ b/packages/core/lib/selector-is.ts @@ -0,0 +1,4 @@ +import { ItemSelector } from "./get-item"; + +export const selectorIs = (a: ItemSelector | null, b: ItemSelector | null) => + a?.zone === b?.zone && a?.index === b?.index; diff --git a/packages/core/lib/use-on-value-change.ts b/packages/core/lib/use-on-value-change.ts new file mode 100644 index 0000000000..3f7c3313f8 --- /dev/null +++ b/packages/core/lib/use-on-value-change.ts @@ -0,0 +1,19 @@ +import { useRef, useEffect } from "react"; + +export function useOnValueChange( + value: T, + onChange: (value: T, oldValue: T) => void, + compare: (value: T, oldValue: T) => boolean = Object.is, + deps: any[] = [] +) { + const tracked = useRef(value); + + useEffect(() => { + const oldValue = tracked.current; + + if (!compare(value, oldValue)) { + tracked.current = value; + onChange(value, oldValue); + } + }, [onChange, compare, value, ...deps]); +} diff --git a/packages/core/lib/use-parent.ts b/packages/core/lib/use-parent.ts index 6934e1e53e..692cd7641c 100644 --- a/packages/core/lib/use-parent.ts +++ b/packages/core/lib/use-parent.ts @@ -1,19 +1,39 @@ -import { useContext } from "react"; +import { useCallback, useContext } from "react"; import { useAppContext } from "../components/Puck/context"; import { getItem, ItemSelector } from "./get-item"; import { dropZoneContext } from "../components/DropZone"; import { convertPathDataToBreadcrumbs } from "./use-breadcrumbs"; +import { PathData } from "../components/DropZone/context"; +import { Data } from "../types"; -export const useParent = (itemSelector?: ItemSelector) => { - const { selectedItem, state } = useAppContext(); - const { pathData } = useContext(dropZoneContext) || {}; - const item = itemSelector ? getItem(itemSelector, state.data) : selectedItem; - const breadcrumbs = convertPathDataToBreadcrumbs(item, pathData, state.data); +export const getParent = ( + itemSelector: ItemSelector | null, + pathData: PathData | undefined, + data: Data +) => { + if (!itemSelector) return null; + + const item = getItem(itemSelector, data); + const breadcrumbs = convertPathDataToBreadcrumbs(item, pathData, data); const lastItem = breadcrumbs[breadcrumbs.length - 1]; const parent = lastItem?.selector - ? getItem(lastItem.selector, state.data) ?? null + ? getItem(lastItem.selector, data) ?? null : null; return parent || null; }; + +export const useGetParent = () => { + const { state } = useAppContext(); + const { pathData } = useContext(dropZoneContext) || {}; + + return useCallback( + () => getParent(state.ui.itemSelector, pathData, state.data), + [state.ui.itemSelector, pathData, state.data] + ); +}; + +export const useParent = () => { + return useGetParent()(); +}; diff --git a/packages/core/lib/use-resolved-fields.ts b/packages/core/lib/use-resolved-fields.ts new file mode 100644 index 0000000000..495ed574d7 --- /dev/null +++ b/packages/core/lib/use-resolved-fields.ts @@ -0,0 +1,157 @@ +import { ComponentData, RootData } from "../types"; +import type { Field, Fields as FieldsType } from "../types"; +import { useAppContext } from "../components/Puck/context"; + +import { useCallback, useEffect, useMemo, useRef, useState } from "react"; +import { ItemSelector } from "../lib/get-item"; +import { getChanged } from "../lib/get-changed"; +import { useParent } from "../lib/use-parent"; +import { useOnValueChange } from "../lib/use-on-value-change"; +import { selectorIs } from "../lib/selector-is"; + +const defaultPageFields: Record = { + title: { type: "text" }, +}; + +type ComponentOrRootData = Omit, "type">; + +export const useResolvedFields = (): [FieldsType, boolean] => { + const { selectedItem, state, config } = useAppContext(); + const parent = useParent(); + + const { data } = state; + + const rootFields = config.root?.fields || defaultPageFields; + + const componentConfig = selectedItem + ? config.components[selectedItem.type] + : null; + + const defaultFields = useMemo( + () => + (selectedItem + ? (componentConfig?.fields as Record>) + : rootFields) || {}, + [selectedItem, rootFields, componentConfig?.fields] + ); + + // DEPRECATED + const rootProps = data.root.props || data.root; + + const [lastSelectedData, setLastSelectedData] = useState< + Partial + >({}); + const [resolvedFields, setResolvedFields] = useState(defaultFields); + const [fieldsLoading, setFieldsLoading] = useState(false); + const lastFields = useRef(defaultFields); + + const defaultResolveFields = ( + _componentData: ComponentOrRootData, + _params: { + fields: FieldsType; + lastData: Partial | null; + lastFields: FieldsType; + changed: Record; + } + ) => defaultFields; + + const componentData: ComponentOrRootData = selectedItem + ? selectedItem + : { props: rootProps, readOnly: data.root.readOnly }; + + const hasComponentResolver = selectedItem && componentConfig?.resolveFields; + const hasRootResolver = !selectedItem && config.root?.resolveFields; + const hasResolver = hasComponentResolver || hasRootResolver; + + const resolveFields = useCallback( + async (fields: FieldsType = {}) => { + const lastData = + lastSelectedData.props?.id === componentData.props.id + ? lastSelectedData + : null; + + const changed = getChanged(componentData, lastData); + + setLastSelectedData(componentData); + + if (hasComponentResolver) { + return await componentConfig!.resolveFields!( + componentData as ComponentData, + { + changed, + fields, + lastFields: lastFields.current, + lastData: lastData as ComponentData, + appState: state, + parent, + } + ); + } + + if (hasRootResolver) { + return await config.root!.resolveFields!(componentData, { + changed, + fields, + lastFields: lastFields.current, + lastData: lastData as RootData, + appState: state, + parent, + }); + } + + return defaultResolveFields(componentData, { + changed, + fields, + lastFields: lastFields.current, + lastData, + }); + }, + [data, config, componentData, selectedItem, state, parent] + ); + + const triggerResolver = useCallback(() => { + // Must either be in default zone, or have parent + if ( + !state.ui.itemSelector?.zone || + state.ui.itemSelector?.zone === "default-zone" || + parent + ) { + if (hasResolver) { + setFieldsLoading(true); + + resolveFields(defaultFields).then((fields) => { + setResolvedFields(fields || {}); + + lastFields.current = fields; + + setFieldsLoading(false); + }); + + return; + } + } + setResolvedFields(defaultFields); + }, [defaultFields, state.ui.itemSelector, hasResolver, parent]); + + useOnValueChange( + state.ui.itemSelector, + () => { + lastFields.current = defaultFields; + }, + selectorIs + ); + + useOnValueChange( + { data, parent, itemSelector: state.ui.itemSelector }, + () => { + triggerResolver(); + }, + (a, b) => JSON.stringify(a) === JSON.stringify(b) + ); + + useEffect(() => { + triggerResolver(); + }, []); + + return [resolvedFields, fieldsLoading]; +};