From 3ea1d393fd45873b19359c8fb4d9b54215d3906c Mon Sep 17 00:00:00 2001 From: James Mikrut Date: Fri, 3 Jan 2025 14:55:52 -0500 Subject: [PATCH] fix(ui): properly reflects hook changes in ui (#10268) Fixes #9882 and #9691 In 2.0, we would accept data coming back from an update operation and then reflect those changes in UI. However, in 3.0, we did not do that anymore - meaning you could change a document with hooks in `beforeChange` or `afterChange`, but then not see the changes made on the server. This PR updates the way that `mergeServerFormState` works, and adds a property to optionally allow values from server form state - which can then be used in the `onSuccess` form handler which may need to define new field values. --- packages/ui/src/fields/Text/index.tsx | 2 +- packages/ui/src/forms/Form/index.tsx | 17 +++++---- .../ui/src/forms/Form/mergeServerFormState.ts | 35 +++++++++++------ packages/ui/src/forms/useField/index.tsx | 6 +-- packages/ui/src/views/Edit/index.tsx | 38 +++++++++---------- test/hooks/collections/BeforeChange/index.ts | 22 +++++++++++ test/hooks/config.ts | 2 + test/hooks/e2e.spec.ts | 14 +++++++ test/hooks/payload-types.ts | 25 ++++++++++++ 9 files changed, 117 insertions(+), 44 deletions(-) create mode 100644 test/hooks/collections/BeforeChange/index.ts diff --git a/packages/ui/src/fields/Text/index.tsx b/packages/ui/src/fields/Text/index.tsx index 8af94859e..28f1b68d9 100644 --- a/packages/ui/src/fields/Text/index.tsx +++ b/packages/ui/src/fields/Text/index.tsx @@ -12,8 +12,8 @@ import { useConfig } from '../../providers/Config/index.js' import { useLocale } from '../../providers/Locale/index.js' import { mergeFieldStyles } from '../mergeFieldStyles.js' import { isFieldRTL } from '../shared/index.js' -import './index.scss' import { TextInput } from './Input.js' +import './index.scss' export { TextInput, TextInputProps } diff --git a/packages/ui/src/forms/Form/index.tsx b/packages/ui/src/forms/Form/index.tsx index 77e563e79..5219d4abb 100644 --- a/packages/ui/src/forms/Form/index.tsx +++ b/packages/ui/src/forms/Form/index.tsx @@ -334,10 +334,11 @@ export const Form: React.FC = (props) => { if (typeof onSuccess === 'function') { const newFormState = await onSuccess(json) if (newFormState) { - const { newState: mergedFormState } = mergeServerFormState( - contextRef.current.fields || {}, - newFormState, - ) + const { newState: mergedFormState } = mergeServerFormState({ + acceptValues: true, + existingState: contextRef.current.fields || {}, + incomingState: newFormState, + }) dispatchFields({ type: 'REPLACE_STATE', @@ -666,10 +667,10 @@ export const Form: React.FC = (props) => { return } - const { changed, newState } = mergeServerFormState( - contextRef.current.fields || {}, - revalidatedFormState, - ) + const { changed, newState } = mergeServerFormState({ + existingState: contextRef.current.fields || {}, + incomingState: revalidatedFormState, + }) if (changed) { dispatchFields({ diff --git a/packages/ui/src/forms/Form/mergeServerFormState.ts b/packages/ui/src/forms/Form/mergeServerFormState.ts index ad1e8e09f..1a6107a97 100644 --- a/packages/ui/src/forms/Form/mergeServerFormState.ts +++ b/packages/ui/src/forms/Form/mergeServerFormState.ts @@ -4,14 +4,11 @@ import { type FormState } from 'payload' import { mergeErrorPaths } from './mergeErrorPaths.js' -const serverPropsToAccept = [ - 'passesCondition', - 'valid', - 'errorMessage', - 'rows', - 'customComponents', - 'requiresRender', -] +type Args = { + acceptValues?: boolean + existingState: FormState + incomingState: FormState +} /** * Merges certain properties from the server state into the client state. These do not include values, @@ -20,10 +17,24 @@ const serverPropsToAccept = [ * We want to use this to update the error state, and other properties that are not user input, as the error state * is the thing we want to keep in sync with the server (where it's calculated) on the client. */ -export const mergeServerFormState = ( - existingState: FormState, - incomingState: FormState, -): { changed: boolean; newState: FormState } => { +export const mergeServerFormState = ({ + acceptValues, + existingState, + incomingState, +}: Args): { changed: boolean; newState: FormState } => { + const serverPropsToAccept = [ + 'passesCondition', + 'valid', + 'errorMessage', + 'rows', + 'customComponents', + 'requiresRender', + ] + + if (acceptValues) { + serverPropsToAccept.push('value') + } + let changed = false const newState = {} diff --git a/packages/ui/src/forms/useField/index.tsx b/packages/ui/src/forms/useField/index.tsx index 5f79710fd..9c12c33ab 100644 --- a/packages/ui/src/forms/useField/index.tsx +++ b/packages/ui/src/forms/useField/index.tsx @@ -38,10 +38,8 @@ export const useField = (options: Options): FieldType => { const { id, collectionSlug } = useDocumentInfo() const operation = useOperation() - const { dispatchField, field } = useFormFields(([fields, dispatch]) => ({ - dispatchField: dispatch, - field: (fields && fields?.[path]) || null, - })) + const dispatchField = useFormFields(([_, dispatch]) => dispatch) + const field = useFormFields(([fields]) => (fields && fields?.[path]) || null) const { t } = useTranslation() const { config } = useConfig() diff --git a/packages/ui/src/views/Edit/index.tsx b/packages/ui/src/views/Edit/index.tsx index 83f2ee63b..41c0295ef 100644 --- a/packages/ui/src/views/Edit/index.tsx +++ b/packages/ui/src/views/Edit/index.tsx @@ -291,32 +291,32 @@ export const DefaultEditView: React.FC = ({ } }, [ - adminRoute, - collectionSlug, - depth, - docPermissions, + reportUpdate, + id, entitySlug, + user, + collectionSlug, + userSlug, + incrementVersionCount, + updateSavedDocumentData, + onSaveFromContext, + isEditing, + depth, getDocPermissions, + globalSlug, + autosaveEnabled, + refreshCookieAsync, + adminRoute, + locale, + router, + resetUploadEdits, getDocPreferences, getFormState, - globalSlug, - id, - incrementVersionCount, - isEditing, - isLockingEnabled, - locale, - onSaveFromContext, + docPermissions, operation, - refreshCookieAsync, - reportUpdate, - resetUploadEdits, - router, schemaPathSegments, + isLockingEnabled, setDocumentIsLocked, - updateSavedDocumentData, - user, - userSlug, - autosaveEnabled, ], ) diff --git a/test/hooks/collections/BeforeChange/index.ts b/test/hooks/collections/BeforeChange/index.ts new file mode 100644 index 000000000..277fdfef9 --- /dev/null +++ b/test/hooks/collections/BeforeChange/index.ts @@ -0,0 +1,22 @@ +import type { CollectionConfig } from 'payload' + +export const BeforeChangeHooks: CollectionConfig = { + slug: 'before-change-hooks', + hooks: { + beforeChange: [ + ({ data }) => { + data.title = 'hi from hook' + + return data + }, + ], + }, + fields: [ + { + name: 'title', + label: 'Title', + type: 'text', + required: true, + }, + ], +} diff --git a/test/hooks/config.ts b/test/hooks/config.ts index aa90331ef..d6699cd88 100644 --- a/test/hooks/config.ts +++ b/test/hooks/config.ts @@ -8,6 +8,7 @@ import { APIError } from 'payload' import { buildConfigWithDefaults } from '../buildConfigWithDefaults.js' import { AfterOperationCollection } from './collections/AfterOperation/index.js' +import { BeforeChangeHooks } from './collections/BeforeChange/index.js' import { BeforeValidateCollection } from './collections/BeforeValidate/index.js' import ChainingHooks from './collections/ChainingHooks/index.js' import ContextHooks from './collections/ContextHooks/index.js' @@ -25,6 +26,7 @@ export const HooksConfig: Promise = buildConfigWithDefaults({ }, }, collections: [ + BeforeChangeHooks, BeforeValidateCollection, AfterOperationCollection, ContextHooks, diff --git a/test/hooks/e2e.spec.ts b/test/hooks/e2e.spec.ts index 229ce34c6..ef9a68c56 100644 --- a/test/hooks/e2e.spec.ts +++ b/test/hooks/e2e.spec.ts @@ -23,6 +23,7 @@ let payload: PayloadTestSDK describe('Hooks', () => { let url: AdminUrlUtil + let beforeChangeURL: AdminUrlUtil let page: Page let serverURL: string @@ -31,6 +32,7 @@ describe('Hooks', () => { ;({ payload, serverURL } = await initPayloadE2ENoConfig({ dirname })) url = new AdminUrlUtil(serverURL, 'before-validate') + beforeChangeURL = new AdminUrlUtil(serverURL, 'before-change-hooks') const context = await browser.newContext() page = await context.newPage() @@ -58,6 +60,18 @@ describe('Hooks', () => { await expect(page.locator('#field-title')).toHaveValue('reset in beforeValidate') }) + + test('should reflect changes made in beforeChange collection hooks within ui after save', async () => { + await page.goto(beforeChangeURL.create) + await page.locator('#field-title').fill('should replace value with before change response') + await saveDocAndAssert(page) + + await expect(page.locator('#field-title')).toHaveValue('hi from hook') + await page.locator('#field-title').fill('helllooooooooo') + await saveDocAndAssert(page) + + await expect(page.locator('#field-title')).toHaveValue('hi from hook') + }) }) async function clearAllDocs(): Promise { diff --git a/test/hooks/payload-types.ts b/test/hooks/payload-types.ts index 292ce47f1..7427e2bb2 100644 --- a/test/hooks/payload-types.ts +++ b/test/hooks/payload-types.ts @@ -11,6 +11,7 @@ export interface Config { 'hooks-users': HooksUserAuthOperations; }; collections: { + 'before-change-hooks': BeforeChangeHook; 'before-validate': BeforeValidate; afterOperation: AfterOperation; 'context-hooks': ContextHook; @@ -27,6 +28,7 @@ export interface Config { }; collectionsJoins: {}; collectionsSelect: { + 'before-change-hooks': BeforeChangeHooksSelect | BeforeChangeHooksSelect; 'before-validate': BeforeValidateSelect | BeforeValidateSelect; afterOperation: AfterOperationSelect | AfterOperationSelect; 'context-hooks': ContextHooksSelect | ContextHooksSelect; @@ -77,6 +79,16 @@ export interface HooksUserAuthOperations { password: string; }; } +/** + * This interface was referenced by `Config`'s JSON-Schema + * via the `definition` "before-change-hooks". + */ +export interface BeforeChangeHook { + id: string; + title: string; + updatedAt: string; + createdAt: string; +} /** * This interface was referenced by `Config`'s JSON-Schema * via the `definition` "before-validate". @@ -231,6 +243,10 @@ export interface DataHook { export interface PayloadLockedDocument { id: string; document?: + | ({ + relationTo: 'before-change-hooks'; + value: string | BeforeChangeHook; + } | null) | ({ relationTo: 'before-validate'; value: string | BeforeValidate; @@ -313,6 +329,15 @@ export interface PayloadMigration { updatedAt: string; createdAt: string; } +/** + * This interface was referenced by `Config`'s JSON-Schema + * via the `definition` "before-change-hooks_select". + */ +export interface BeforeChangeHooksSelect { + title?: T; + updatedAt?: T; + createdAt?: T; +} /** * This interface was referenced by `Config`'s JSON-Schema * via the `definition` "before-validate_select".