From dc9e8fa6551b4792ce00974e09a8df30226a535c Mon Sep 17 00:00:00 2001 From: Alessio Gravili Date: Mon, 24 Feb 2025 11:37:33 -0700 Subject: [PATCH] refactor: simplify running field hooks (#11372) Previously, we were quite frequently using `.reduce()` to sequentially run field hooks. This PR replaces them with simple `for` loops, which is less overhead, less code, less confusing and simpler to understand. Additionally, it refactors `mergeLocaleActions` which previously was unnecessarily complex. They no longer entail async code, thus we no longer have to juggle with promises --- packages/payload/src/admin/RichText.ts | 2 +- .../src/fields/hooks/afterChange/promise.ts | 20 +++--- .../src/fields/hooks/afterRead/promise.ts | 41 +++++------ .../src/fields/hooks/beforeChange/index.ts | 5 +- .../src/fields/hooks/beforeChange/promise.ts | 50 ++++++-------- .../hooks/beforeChange/traverseFields.ts | 2 +- .../fields/hooks/beforeDuplicate/promise.ts | 68 +++++++++---------- .../fields/hooks/beforeDuplicate/runHook.ts | 8 --- .../fields/hooks/beforeValidate/promise.ts | 16 ++--- .../src/features/typesServer.ts | 2 +- 10 files changed, 89 insertions(+), 125 deletions(-) delete mode 100644 packages/payload/src/fields/hooks/beforeDuplicate/runHook.ts diff --git a/packages/payload/src/admin/RichText.ts b/packages/payload/src/admin/RichText.ts index 6812ba569..f5cacf4c1 100644 --- a/packages/payload/src/admin/RichText.ts +++ b/packages/payload/src/admin/RichText.ts @@ -88,7 +88,7 @@ export type BeforeChangeRichTextHookArgs< errors?: ValidationFieldError[] /** Only available in `beforeChange` field hooks */ - mergeLocaleActions?: (() => Promise)[] + mergeLocaleActions?: (() => Promise | void)[] /** A string relating to which operation the field type is currently executing within. */ operation?: 'create' | 'delete' | 'read' | 'update' /** The sibling data of the document before changes being applied. */ diff --git a/packages/payload/src/fields/hooks/afterChange/promise.ts b/packages/payload/src/fields/hooks/afterChange/promise.ts index 23b1959e6..dc18f307d 100644 --- a/packages/payload/src/fields/hooks/afterChange/promise.ts +++ b/packages/payload/src/fields/hooks/afterChange/promise.ts @@ -75,10 +75,8 @@ export const promise = async ({ if (fieldAffectsData(field)) { // Execute hooks if (field.hooks?.afterChange) { - await field.hooks.afterChange.reduce(async (priorHook, currentHook) => { - await priorHook - - const hookedValue = await currentHook({ + for (const hook of field.hooks.afterChange) { + const hookedValue = await hook({ blockData, collection, context, @@ -102,7 +100,7 @@ export const promise = async ({ if (hookedValue !== undefined) { siblingDoc[field.name] = hookedValue } - }, Promise.resolve()) + } } } @@ -242,17 +240,15 @@ export const promise = async ({ throw new MissingEditorProp(field) // while we allow disabling editor functionality, you should not have any richText fields defined if you do not have an editor } - if (typeof field?.editor === 'function') { + if (typeof field.editor === 'function') { throw new Error('Attempted to access unsanitized rich text editor.') } - const editor: RichTextAdapter = field?.editor + const editor: RichTextAdapter = field.editor if (editor?.hooks?.afterChange?.length) { - await editor.hooks.afterChange.reduce(async (priorHook, currentHook) => { - await priorHook - - const hookedValue = await currentHook({ + for (const hook of editor.hooks.afterChange) { + const hookedValue = await hook({ collection, context, data, @@ -275,7 +271,7 @@ export const promise = async ({ if (hookedValue !== undefined) { siblingDoc[field.name] = hookedValue } - }, Promise.resolve()) + } } break } diff --git a/packages/payload/src/fields/hooks/afterRead/promise.ts b/packages/payload/src/fields/hooks/afterRead/promise.ts index df3da5dab..bbc2787ee 100644 --- a/packages/payload/src/fields/hooks/afterRead/promise.ts +++ b/packages/payload/src/fields/hooks/afterRead/promise.ts @@ -237,18 +237,17 @@ export const promise = async ({ if (fieldAffectsData(field)) { // Execute hooks if (triggerHooks && field.hooks?.afterRead) { - await field.hooks.afterRead.reduce(async (priorHook, currentHook) => { - await priorHook - + for (const hook of field.hooks.afterRead) { const shouldRunHookOnAllLocales = fieldShouldBeLocalized({ field, parentIsLocalized }) && (locale === 'all' || !flattenLocales) && typeof siblingDoc[field.name] === 'object' if (shouldRunHookOnAllLocales) { - const hookPromises = Object.entries(siblingDoc[field.name]).map(([locale, value]) => - (async () => { - const hookedValue = await currentHook({ + const localesAndValues = Object.entries(siblingDoc[field.name]) + await Promise.all( + localesAndValues.map(async ([localeKey, value]) => { + const hookedValue = await hook({ blockData, collection, context, @@ -273,14 +272,12 @@ export const promise = async ({ }) if (hookedValue !== undefined) { - siblingDoc[field.name][locale] = hookedValue + siblingDoc[field.name][localeKey] = hookedValue } - })(), + }), ) - - await Promise.all(hookPromises) } else { - const hookedValue = await currentHook({ + const hookedValue = await hook({ blockData, collection, context, @@ -308,7 +305,7 @@ export const promise = async ({ siblingDoc[field.name] = hookedValue } } - }, Promise.resolve()) + } } // Execute access control @@ -677,18 +674,18 @@ export const promise = async ({ const editor: RichTextAdapter = field?.editor if (editor?.hooks?.afterRead?.length) { - await editor.hooks.afterRead.reduce(async (priorHook, currentHook) => { - await priorHook - + for (const hook of editor.hooks.afterRead) { const shouldRunHookOnAllLocales = fieldShouldBeLocalized({ field, parentIsLocalized }) && (locale === 'all' || !flattenLocales) && typeof siblingDoc[field.name] === 'object' if (shouldRunHookOnAllLocales) { - const hookPromises = Object.entries(siblingDoc[field.name]).map(([locale, value]) => - (async () => { - const hookedValue = await currentHook({ + const localesAndValues = Object.entries(siblingDoc[field.name]) + + await Promise.all( + localesAndValues.map(async ([locale, value]) => { + const hookedValue = await hook({ collection, context, currentDepth, @@ -722,12 +719,10 @@ export const promise = async ({ if (hookedValue !== undefined) { siblingDoc[field.name][locale] = hookedValue } - })(), + }), ) - - await Promise.all(hookPromises) } else { - const hookedValue = await currentHook({ + const hookedValue = await hook({ collection, context, currentDepth, @@ -762,7 +757,7 @@ export const promise = async ({ siblingDoc[field.name] = hookedValue } } - }, Promise.resolve()) + } } break } diff --git a/packages/payload/src/fields/hooks/beforeChange/index.ts b/packages/payload/src/fields/hooks/beforeChange/index.ts index 3738984f1..4631d2b4e 100644 --- a/packages/payload/src/fields/hooks/beforeChange/index.ts +++ b/packages/payload/src/fields/hooks/beforeChange/index.ts @@ -81,10 +81,9 @@ export const beforeChange = async ({ ) } - await mergeLocaleActions.reduce(async (priorAction, action) => { - await priorAction + for (const action of mergeLocaleActions) { await action() - }, Promise.resolve()) + } return data } diff --git a/packages/payload/src/fields/hooks/beforeChange/promise.ts b/packages/payload/src/fields/hooks/beforeChange/promise.ts index cf284c231..6dafbd014 100644 --- a/packages/payload/src/fields/hooks/beforeChange/promise.ts +++ b/packages/payload/src/fields/hooks/beforeChange/promise.ts @@ -31,7 +31,7 @@ type Args = { fieldIndex: number global: null | SanitizedGlobalConfig id?: number | string - mergeLocaleActions: (() => Promise)[] + mergeLocaleActions: (() => Promise | void)[] operation: Operation parentIndexPath: string parentIsLocalized: boolean @@ -108,10 +108,8 @@ export const promise = async ({ // Execute hooks if (field.hooks?.beforeChange) { - await field.hooks.beforeChange.reduce(async (priorHook, currentHook) => { - await priorHook - - const hookedValue = await currentHook({ + for (const hook of field.hooks.beforeChange) { + const hookedValue = await hook({ blockData, collection, context, @@ -135,7 +133,7 @@ export const promise = async ({ if (hookedValue !== undefined) { siblingData[field.name] = hookedValue } - }, Promise.resolve()) + } } // Validate @@ -192,28 +190,20 @@ export const promise = async ({ // Push merge locale action if applicable if (localization && fieldShouldBeLocalized({ field, parentIsLocalized })) { - mergeLocaleActions.push(async () => { - const localeData = await localization.localeCodes.reduce( - async (localizedValuesPromise: Promise, locale) => { - const localizedValues = await localizedValuesPromise - const fieldValue = - locale === req.locale - ? siblingData[field.name] - : siblingDocWithLocales?.[field.name]?.[locale] + mergeLocaleActions.push(() => { + const localeData = {} - // const result = await localizedValues - // update locale value if it's not undefined - if (typeof fieldValue !== 'undefined') { - return { - ...localizedValues, - [locale]: fieldValue, - } - } + for (const locale of localization.localeCodes) { + const fieldValue = + locale === req.locale + ? siblingData[field.name] + : siblingDocWithLocales?.[field.name]?.[locale] - return localizedValuesPromise - }, - Promise.resolve({}), - ) + // update locale value if it's not undefined + if (typeof fieldValue !== 'undefined') { + localeData[locale] = fieldValue + } + } // If there are locales with data, set the data if (Object.keys(localeData).length > 0) { @@ -423,10 +413,8 @@ export const promise = async ({ const editor: RichTextAdapter = field?.editor if (editor?.hooks?.beforeChange?.length) { - await editor.hooks.beforeChange.reduce(async (priorHook, currentHook) => { - await priorHook - - const hookedValue = await currentHook({ + for (const hook of editor.hooks.beforeChange) { + const hookedValue = await hook({ collection, context, data, @@ -453,7 +441,7 @@ export const promise = async ({ if (hookedValue !== undefined) { siblingData[field.name] = hookedValue } - }, Promise.resolve()) + } } break diff --git a/packages/payload/src/fields/hooks/beforeChange/traverseFields.ts b/packages/payload/src/fields/hooks/beforeChange/traverseFields.ts index abde171bd..84c9eabf9 100644 --- a/packages/payload/src/fields/hooks/beforeChange/traverseFields.ts +++ b/packages/payload/src/fields/hooks/beforeChange/traverseFields.ts @@ -28,7 +28,7 @@ type Args = { fields: (Field | TabAsField)[] global: null | SanitizedGlobalConfig id?: number | string - mergeLocaleActions: (() => Promise)[] + mergeLocaleActions: (() => Promise | void)[] operation: Operation parentIndexPath: string /** diff --git a/packages/payload/src/fields/hooks/beforeDuplicate/promise.ts b/packages/payload/src/fields/hooks/beforeDuplicate/promise.ts index b289167bc..2daa2a91b 100644 --- a/packages/payload/src/fields/hooks/beforeDuplicate/promise.ts +++ b/packages/payload/src/fields/hooks/beforeDuplicate/promise.ts @@ -6,7 +6,6 @@ import type { Block, Field, FieldHookArgs, TabAsField } from '../../config/types import { fieldAffectsData, fieldShouldBeLocalized } from '../../config/types.js' import { getFieldPathsModified as getFieldPaths } from '../../getFieldPaths.js' -import { runBeforeDuplicateHooks } from './runHook.js' import { traverseFields } from './traverseFields.js' type Args = { @@ -68,42 +67,37 @@ export const promise = async ({ // Run field beforeDuplicate hooks if (Array.isArray(field.hooks?.beforeDuplicate)) { if (fieldIsLocalized) { - const localeData = await localization.localeCodes.reduce( - async (localizedValuesPromise: Promise, locale) => { - const localizedValues = await localizedValuesPromise + const localeData: JsonObject = {} - const beforeDuplicateArgs: FieldHookArgs = { - blockData, - collection, - context, - data: doc, - field, - global: undefined, - indexPath: indexPathSegments, - path: pathSegments, - previousSiblingDoc: siblingDoc, - previousValue: siblingDoc[field.name]?.[locale], - req, - schemaPath: schemaPathSegments, - siblingData: siblingDoc, - siblingDocWithLocales: siblingDoc, - siblingFields, - value: siblingDoc[field.name]?.[locale], - } + for (const locale of localization.localeCodes) { + const beforeDuplicateArgs: FieldHookArgs = { + blockData, + collection, + context, + data: doc, + field, + global: undefined, + indexPath: indexPathSegments, + path: pathSegments, + previousSiblingDoc: siblingDoc, + previousValue: siblingDoc[field.name]?.[locale], + req, + schemaPath: schemaPathSegments, + siblingData: siblingDoc, + siblingDocWithLocales: siblingDoc, + siblingFields, + value: siblingDoc[field.name]?.[locale], + } - const hookResult = await runBeforeDuplicateHooks(beforeDuplicateArgs) + let hookResult + for (const hook of field.hooks.beforeDuplicate) { + hookResult = await hook(beforeDuplicateArgs) + } - if (typeof hookResult !== 'undefined') { - return { - ...localizedValues, - [locale]: hookResult, - } - } - - return localizedValuesPromise - }, - Promise.resolve({}), - ) + if (typeof hookResult !== 'undefined') { + localeData[locale] = hookResult + } + } siblingDoc[field.name] = localeData } else { @@ -126,7 +120,11 @@ export const promise = async ({ value: siblingDoc[field.name], } - const hookResult = await runBeforeDuplicateHooks(beforeDuplicateArgs) + let hookResult + for (const hook of field.hooks.beforeDuplicate) { + hookResult = await hook(beforeDuplicateArgs) + } + if (typeof hookResult !== 'undefined') { siblingDoc[field.name] = hookResult } diff --git a/packages/payload/src/fields/hooks/beforeDuplicate/runHook.ts b/packages/payload/src/fields/hooks/beforeDuplicate/runHook.ts deleted file mode 100644 index e6232ec4c..000000000 --- a/packages/payload/src/fields/hooks/beforeDuplicate/runHook.ts +++ /dev/null @@ -1,8 +0,0 @@ -// @ts-strict-ignore -import type { FieldHookArgs } from '../../config/types.js' - -export const runBeforeDuplicateHooks = async (args: FieldHookArgs) => - await args.field.hooks.beforeDuplicate.reduce(async (priorHook, currentHook) => { - await priorHook - return await currentHook(args) - }, Promise.resolve()) diff --git a/packages/payload/src/fields/hooks/beforeValidate/promise.ts b/packages/payload/src/fields/hooks/beforeValidate/promise.ts index 5917f6db8..59ba4a8a7 100644 --- a/packages/payload/src/fields/hooks/beforeValidate/promise.ts +++ b/packages/payload/src/fields/hooks/beforeValidate/promise.ts @@ -276,10 +276,8 @@ export const promise = async ({ // Execute hooks if (field.hooks?.beforeValidate) { - await field.hooks.beforeValidate.reduce(async (priorHook, currentHook) => { - await priorHook - - const hookedValue = await currentHook({ + for (const hook of field.hooks.beforeValidate) { + const hookedValue = await hook({ blockData, collection, context, @@ -303,7 +301,7 @@ export const promise = async ({ if (hookedValue !== undefined) { siblingData[field.name] = hookedValue } - }, Promise.resolve()) + } } // Execute access control @@ -493,10 +491,8 @@ export const promise = async ({ const editor: RichTextAdapter = field?.editor if (editor?.hooks?.beforeValidate?.length) { - await editor.hooks.beforeValidate.reduce(async (priorHook, currentHook) => { - await priorHook - - const hookedValue = await currentHook({ + for (const hook of editor.hooks.beforeValidate) { + const hookedValue = await hook({ collection, context, data, @@ -519,7 +515,7 @@ export const promise = async ({ if (hookedValue !== undefined) { siblingData[field.name] = hookedValue } - }, Promise.resolve()) + } } break } diff --git a/packages/richtext-lexical/src/features/typesServer.ts b/packages/richtext-lexical/src/features/typesServer.ts index 35ee04c64..96505597f 100644 --- a/packages/richtext-lexical/src/features/typesServer.ts +++ b/packages/richtext-lexical/src/features/typesServer.ts @@ -176,7 +176,7 @@ export type BeforeChangeNodeHookArgs = { * Only available in `beforeChange` hooks. */ errors: ValidationFieldError[] - mergeLocaleActions: (() => Promise)[] + mergeLocaleActions: (() => Promise | void)[] /** A string relating to which operation the field type is currently executing within. Useful within beforeValidate, beforeChange, and afterChange hooks to differentiate between create and update operations. */ operation: 'create' | 'delete' | 'read' | 'update' /** The value of the node before any changes. Not available in afterRead hooks */