Skip to content

Commit

Permalink
fix(core): Get hooks to work reliably with custom methods (#2714)
Browse files Browse the repository at this point in the history
  • Loading branch information
@daffl
daffl committed Aug 3, 2022
1 parent 3bb4bf4 commit 8d7e04a
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 87 deletions.
131 changes: 58 additions & 73 deletions packages/feathers/src/hooks.ts
View file
Expand Up @@ -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<typeof convertHookData> }

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) {
Expand All @@ -41,80 +55,25 @@ export function convertHookData(input: any) {
return result
}

type HookTypes = 'before' | 'after' | 'error' | 'around'

type ConvertedMap = { [type in HookTypes]: ReturnType<typeof convertHookData> }

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<any, any>, 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', {
Expand All @@ -123,11 +82,37 @@ export function enableHooks(object: any, methods: string[] = defaultServiceMetho
writable: true
})

return function registerHooks(this: any, input: HookMap<any, any>) {
return function registerHooks(this: HookEnabled, input: HookMap<any, any>) {
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
}
Expand All @@ -150,7 +135,7 @@ export class FeathersHookManager<A> 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)

Expand Down Expand Up @@ -200,7 +185,7 @@ export function hookMixin<A>(this: A, service: FeathersService<A>, path: string,
return res
}, {} as BaseHookMap)

const registerHooks = enableHooks(service, hookMethods)
const registerHooks = enableHooks(service)

hooks(service, serviceMethodHooks)

Expand Down
52 changes: 39 additions & 13 deletions packages/feathers/test/hooks/app.test.ts
View file
Expand Up @@ -15,27 +15,39 @@ interface TodoParams extends Params {
ran: boolean
}

type TodoService = ServiceInterface<Todo, Todo, TodoParams>
type TodoService = ServiceInterface<Todo, Todo, TodoParams> & {
customMethod(data: any, params?: TodoParams): Promise<any>
}

type App = Application<{ todos: TodoService }>

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', () => {
Expand Down Expand Up @@ -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([
Expand All @@ -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 }
})
})
})

Expand Down Expand Up @@ -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({
Expand All @@ -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 () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/feathers/test/hooks/error.test.ts
View file
Expand Up @@ -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 () => {
Expand Down

0 comments on commit 8d7e04a

Please sign in to comment.