From a5552ebf062ef1d2d35605155f69d7c743555125 Mon Sep 17 00:00:00 2001 From: daffl Date: Wed, 3 Aug 2022 07:23:40 -0700 Subject: [PATCH] fix(core): Make hooks reliably work with custom methods --- packages/feathers/src/hooks.ts | 131 +++++++++------------ packages/feathers/test/hooks/app.test.ts | 52 ++++++-- packages/feathers/test/hooks/error.test.ts | 2 +- 3 files changed, 98 insertions(+), 87 deletions(-) diff --git a/packages/feathers/src/hooks.ts b/packages/feathers/src/hooks.ts index 02697b702b..f69bb20059 100644 --- a/packages/feathers/src/hooks.ts +++ b/packages/feathers/src/hooks.ts @@ -16,12 +16,26 @@ import { AroundHookFunction, HookFunction } from './declarations' -import { defaultServiceArguments, defaultServiceMethods, getHookMethods } from './service' +import { defaultServiceArguments, getHookMethods } from './service' -export function collectHooks(target: any, method: string) { - return target.__hooks.hooks[method] || [] +type HookType = 'before' | 'after' | 'error' | 'around' + +type ConvertedMap = { [type in HookType]: ReturnType } + +type HookStore = { + around: { [method: string]: AroundHookFunction[] } + before: { [method: string]: HookFunction[] } + after: { [method: string]: HookFunction[] } + error: { [method: string]: HookFunction[] } + collected: { [method: string]: AroundHookFunction[] } } +type HookEnabled = { __hooks: HookStore } + +const types: HookType[] = ['before', 'after', 'error', 'around'] + +const isType = (value: any): value is HookType => types.includes(value) + // Converts different hook registration formats into the // same internal format export function convertHookData(input: any) { @@ -41,80 +55,25 @@ export function convertHookData(input: any) { return result } -type HookTypes = 'before' | 'after' | 'error' | 'around' - -type ConvertedMap = { [type in HookTypes]: ReturnType } - -type HookStore = { - around: { [method: string]: AroundHookFunction[] } - before: { [method: string]: HookFunction[] } - after: { [method: string]: HookFunction[] } - error: { [method: string]: HookFunction[] } - hooks: { [method: string]: AroundHookFunction[] } -} - -const types: HookTypes[] = ['before', 'after', 'error', 'around'] - -const isType = (value: any): value is HookTypes => types.includes(value) - -const createMap = (input: HookMap, methods: string[]) => { - const map = {} as ConvertedMap - - Object.keys(input).forEach((type) => { - if (!isType(type)) { - throw new Error(`'${type}' is not a valid hook type`) - } - - const data = convertHookData(input[type]) - - Object.keys(data).forEach((method) => { - if (method !== 'all' && !methods.includes(method) && !defaultServiceMethods.includes(method)) { - throw new Error(`'${method}' is not a valid hook method`) - } - }) - - map[type] = data - }) +export function collectHooks(target: HookEnabled, method: string) { + const { collected, around } = target.__hooks - return map + return [ + ...(around.all || []), + ...(around[method] || []), + ...(collected.all || []), + ...(collected[method] || []) + ] as AroundHookFunction[] } -const updateStore = (store: HookStore, map: ConvertedMap) => - Object.keys(store.hooks).forEach((method) => { - Object.keys(map).forEach((key) => { - const type = key as HookTypes - const allHooks = map[type].all || [] - const methodHooks = map[type][method] || [] - - if (allHooks.length || methodHooks.length) { - const list = [...allHooks, ...methodHooks] as any - const hooks = (store[type][method] ||= []) - - hooks.push(...list) - } - }) - - const collected = collect({ - before: store.before[method] || [], - after: store.after[method] || [], - error: store.error[method] || [] - }) - - store.hooks[method] = [...(store.around[method] || []), collected] - }) - // Add `.hooks` functionality to an object -export function enableHooks(object: any, methods: string[] = defaultServiceMethods) { +export function enableHooks(object: any) { const store: HookStore = { around: {}, before: {}, after: {}, error: {}, - hooks: {} - } - - for (const method of methods) { - store.hooks[method] = [] + collected: {} } Object.defineProperty(object, '__hooks', { @@ -123,11 +82,37 @@ export function enableHooks(object: any, methods: string[] = defaultServiceMetho writable: true }) - return function registerHooks(this: any, input: HookMap) { + return function registerHooks(this: HookEnabled, input: HookMap) { const store = this.__hooks - const map = createMap(input, methods) + const map = Object.keys(input).reduce((map, type) => { + if (!isType(type)) { + throw new Error(`'${type}' is not a valid hook type`) + } + + map[type] = convertHookData(input[type]) + + return map + }, {} as ConvertedMap) + const types = Object.keys(map) as HookType[] + + types.forEach((type) => + Object.keys(map[type]).forEach((method) => { + const mapHooks = map[type][method] + const storeHooks: any[] = (store[type][method] ||= []) + + storeHooks.push(...mapHooks) + + if (store.before[method] || store.after[method] || store.error[method]) { + const collected = collect({ + before: store.before[method] || [], + after: store.after[method] || [], + error: store.error[method] || [] + }) - updateStore(store, map) + store.collected[method] = [collected] + } + }) + ) return this } @@ -150,7 +135,7 @@ export class FeathersHookManager extends HookManager { } collectMiddleware(self: any, args: any[]): Middleware[] { - const appHooks = collectHooks(this.app, this.method) + const appHooks = collectHooks(this.app as any as HookEnabled, this.method) const middleware = super.collectMiddleware(self, args) const methodHooks = collectHooks(self, this.method) @@ -200,7 +185,7 @@ export function hookMixin(this: A, service: FeathersService, path: string, return res }, {} as BaseHookMap) - const registerHooks = enableHooks(service, hookMethods) + const registerHooks = enableHooks(service) hooks(service, serviceMethodHooks) diff --git a/packages/feathers/test/hooks/app.test.ts b/packages/feathers/test/hooks/app.test.ts index 3cd5ee17c4..2da944343d 100644 --- a/packages/feathers/test/hooks/app.test.ts +++ b/packages/feathers/test/hooks/app.test.ts @@ -15,7 +15,9 @@ interface TodoParams extends Params { ran: boolean } -type TodoService = ServiceInterface +type TodoService = ServiceInterface & { + customMethod(data: any, params?: TodoParams): Promise +} type App = Application<{ todos: TodoService }> @@ -23,19 +25,29 @@ describe('app.hooks', () => { let app: App beforeEach(() => { - app = feathers().use('todos', { - async get(id: any, params: any) { - if (id === 'error') { - throw new Error('Something went wrong') - } + app = feathers<{ todos: TodoService }>().use( + 'todos', + { + async get(id: any, params: any) { + if (id === 'error') { + throw new Error('Something went wrong') + } - return { id, params } - }, + return { id, params } + }, - async create(data: any, params: any) { - return { data, params } + async create(data: any, params: any) { + return { data, params } + }, + + async customMethod(data: any, params: TodoParams) { + return { data, params } + } + }, + { + methods: ['get', 'create', 'customMethod'] } - }) + ) }) it('app has the .hooks method', () => { @@ -80,7 +92,7 @@ describe('app.hooks', () => { }) describe('app.hooks([ async ])', () => { - it('basic app async hook', async () => { + it('basic app async hook, works with custom method', async () => { const service = app.service('todos') app.hooks([ @@ -106,6 +118,13 @@ describe('app.hooks', () => { data, params: { ran: true } }) + + result = await service.customMethod('custom test') + + assert.deepStrictEqual(result, { + data: 'custom test', + params: { ran: true } + }) }) }) @@ -133,7 +152,7 @@ describe('app.hooks', () => { }) describe('app.hooks({ before })', () => { - it('basic app before hook', async () => { + it('basic app before hook, works with custom method', async () => { const service = app.service('todos') app.hooks({ @@ -158,6 +177,13 @@ describe('app.hooks', () => { data, params: { ran: true } }) + + result = await service.customMethod('custom with before') + + assert.deepStrictEqual(result, { + data: 'custom with before', + params: { ran: true } + }) }) it('app before hooks always run first', async () => { diff --git a/packages/feathers/test/hooks/error.test.ts b/packages/feathers/test/hooks/error.test.ts index ad38b0f863..4c42d266cf 100644 --- a/packages/feathers/test/hooks/error.test.ts +++ b/packages/feathers/test/hooks/error.test.ts @@ -15,7 +15,7 @@ describe('`error` hooks', () => { const s = service as any s.__hooks.error.get = undefined - s.__hooks.hooks.get = [] + s.__hooks.collected.get = [] }) it('basic error hook', async () => {