Skip to content

Commit 4b51ded

Browse files
fratzingerfratzinger
committed
fix: early returns for around hooks
1 parent 8cf2c7b commit 4b51ded

21 files changed

Lines changed: 708 additions & 192 deletions

‎.claude/settings.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,8 @@
1818
"Bash(pnpm run test:unit *)",
1919
"Bash(pnpm run build)",
2020
"Bash(gh release list *)",
21-
"Bash(gh release view *)"
21+
"Bash(gh release view *)",
22+
"Bash(npm run *)"
2223
]
2324
}
2425
}

‎src/hooks/check-multi/check-multi.hook.test.ts‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import { vi } from 'vitest'
12
import type { HookContext } from '@feathersjs/feathers'
23
import { checkMulti } from './check-multi.hook.js'
34
import { MethodNotAllowed } from '@feathersjs/errors'
@@ -138,6 +139,76 @@ describe('checkMulti', function () {
138139
})
139140
})
140141

142+
describe('around hooks', function () {
143+
it("calls next() when 'allowsMulti' is not defined", async function () {
144+
const context = {
145+
service: {},
146+
type: 'around',
147+
method: 'create',
148+
data: [],
149+
} as HookContext
150+
const next = vi.fn()
151+
152+
await checkMulti()(context, next)
153+
154+
expect(next).toHaveBeenCalledOnce()
155+
})
156+
157+
it("calls next() when 'allowsMulti' returns true", async function () {
158+
const context = {
159+
service: { allowsMulti: () => true },
160+
type: 'around',
161+
method: 'create',
162+
data: [],
163+
} as HookContext
164+
const next = vi.fn()
165+
166+
await checkMulti()(context, next)
167+
168+
expect(next).toHaveBeenCalledOnce()
169+
})
170+
171+
it("calls next() for 'find' even when 'allowsMulti' returns false", async function () {
172+
const context = {
173+
service: { allowsMulti: () => false },
174+
type: 'around',
175+
method: 'find',
176+
} as HookContext
177+
const next = vi.fn()
178+
179+
await checkMulti()(context, next)
180+
181+
expect(next).toHaveBeenCalledOnce()
182+
})
183+
184+
it('calls next() when no multi data', async function () {
185+
const context = {
186+
service: { allowsMulti: () => false },
187+
type: 'around',
188+
method: 'create',
189+
data: {},
190+
} as HookContext
191+
const next = vi.fn()
192+
193+
await checkMulti()(context, next)
194+
195+
expect(next).toHaveBeenCalledOnce()
196+
})
197+
198+
it("does not call next() and throws when 'allowsMulti' returns false and multi data", function () {
199+
const context = {
200+
service: { allowsMulti: () => false },
201+
type: 'around',
202+
method: 'create',
203+
data: [],
204+
} as HookContext
205+
const next = vi.fn()
206+
207+
expect(() => checkMulti()(context, next)).toThrow(MethodNotAllowed)
208+
expect(next).not.toHaveBeenCalled()
209+
})
210+
})
211+
141212
it('can skip hook', function () {
142213
const makeContext = (type: string, method: string) => {
143214
const context = {

‎src/hooks/check-multi/check-multi.hook.ts‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -33,20 +33,18 @@ export function checkMulti<H extends HookContext = HookContext>(
3333
) {
3434
return (context: H, next?: NextFunction) => {
3535
const { service, method } = context
36-
if (!service.allowsMulti || !isMulti(context) || method === 'find') {
36+
if (
37+
!service.allowsMulti ||
38+
!isMulti(context) ||
39+
method === 'find' ||
40+
service.allowsMulti(method)
41+
) {
42+
if (next) return next()
3743
return context
3844
}
3945

40-
if (!service.allowsMulti(method)) {
41-
throw options?.error
42-
? options.error(context)
43-
: new MethodNotAllowed(`Can not ${method} multiple entries`)
44-
}
45-
46-
if (next) {
47-
return next()
48-
}
49-
50-
return context
46+
throw options?.error
47+
? options.error(context)
48+
: new MethodNotAllowed(`Can not ${method} multiple entries`)
5149
}
5250
}

‎src/hooks/iff-else/iff-else.hook.test.ts‎

Lines changed: 90 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/* eslint-disable @typescript-eslint/no-this-alias */
22
import type { HookContext } from '@feathersjs/feathers'
3-
import { assert } from 'vitest'
3+
import { assert, expect, vi } from 'vitest'
44
import { iffElse } from './iff-else.hook.js'
55
import { or } from '../../predicates/or/or.predicate.js'
66
import { and } from '../../predicates/and/and.predicate.js'
@@ -53,9 +53,9 @@ const hookFcn = function (this: any, hook: any, _cb: any) {
5353
const predicateTrue = function (
5454
this: any,
5555
hook: any,
56-
more2: any,
57-
more3: any,
58-
more4: any,
56+
more2?: any,
57+
more3?: any,
58+
more4?: any,
5959
): true {
6060
predicateTrueContext = this
6161

@@ -128,28 +128,26 @@ describe('services iffElse', () => {
128128
})
129129

130130
it('when false', () => {
131-
return (
132-
// @ts-expect-error TODO
133-
iffElse(false, null, [hookFcnSync, hookFcnAsync, hookFcn])(hook).then(
134-
(hook: any) => {
135-
assert.deepEqual(hook, hookAfter)
136-
assert.equal(hookFcnSyncCalls, 1)
137-
assert.equal(hookFcnAsyncCalls, 1)
138-
assert.equal(hookFcnCalls, 1)
139-
assert.deepEqual(hook, hookAfter)
140-
},
141-
)
142-
)
131+
return iffElse(
132+
false,
133+
undefined,
134+
[hookFcnSync, hookFcnAsync, hookFcn],
135+
)(hook).then((hook: any) => {
136+
assert.deepEqual(hook, hookAfter)
137+
assert.equal(hookFcnSyncCalls, 1)
138+
assert.equal(hookFcnAsyncCalls, 1)
139+
assert.equal(hookFcnCalls, 1)
140+
assert.deepEqual(hook, hookAfter)
141+
})
143142
})
144143
})
145144

146145
describe('predicate gets right params', () => {
147146
it('when true', () => {
148147
return iffElse(
149-
// @ts-expect-error TODO
150148
predicateTrue,
151149
[hookFcnSync, hookFcnAsync, hookFcn],
152-
null,
150+
undefined,
153151
)(hook).then(() => {
154152
assert.deepEqual(predicateParam1, hook, 'param1')
155153
assert.strictEqual(predicateParam2, undefined, 'param2')
@@ -160,7 +158,6 @@ describe('services iffElse', () => {
160158

161159
it('and passes on correct params', () => {
162160
return iffElse(
163-
// @ts-expect-error TODO
164161
and(predicateTrue),
165162
[hookFcnSync, hookFcnAsync, hookFcn],
166163
[],
@@ -174,7 +171,6 @@ describe('services iffElse', () => {
174171

175172
it('or passes on correct params', () => {
176173
return iffElse(
177-
// @ts-expect-error TODO
178174
or(predicateTrue),
179175
[hookFcnSync, hookFcnAsync, hookFcn],
180176
[],
@@ -197,23 +193,81 @@ describe('services iffElse', () => {
197193
})
198194

199195
it('services', () => {
200-
return (
201-
// @ts-expect-error TODO
202-
iffElse(predicateTrue, [hookFcnSync, hookFcnAsync, hookFcn], null)
203-
.call(context, hook)
204-
.then((hook: any) => {
205-
assert.deepEqual(hook, hookAfter)
206-
assert.equal(hookFcnSyncCalls, 1)
207-
assert.equal(hookFcnAsyncCalls, 1)
208-
assert.equal(hookFcnCalls, 1)
209-
assert.deepEqual(hook, hookAfter)
210-
211-
assert.deepEqual(predicateTrueContext, { service: 'abc' })
212-
assert.deepEqual(hookFcnSyncContext, { service: 'abc' })
213-
assert.deepEqual(hookFcnAsyncContext, { service: 'abc' })
214-
assert.deepEqual(hookFcnContext, { service: 'abc' })
215-
})
196+
return iffElse(
197+
predicateTrue,
198+
[hookFcnSync, hookFcnAsync, hookFcn],
199+
undefined,
216200
)
201+
.call(context, hook)
202+
.then((hook: any) => {
203+
assert.deepEqual(hook, hookAfter)
204+
assert.equal(hookFcnSyncCalls, 1)
205+
assert.equal(hookFcnAsyncCalls, 1)
206+
assert.equal(hookFcnCalls, 1)
207+
assert.deepEqual(hook, hookAfter)
208+
209+
assert.deepEqual(predicateTrueContext, { service: 'abc' })
210+
assert.deepEqual(hookFcnSyncContext, { service: 'abc' })
211+
assert.deepEqual(hookFcnAsyncContext, { service: 'abc' })
212+
assert.deepEqual(hookFcnContext, { service: 'abc' })
213+
})
214+
})
215+
})
216+
217+
describe('around hooks', () => {
218+
it('runs trueHooks and then next() when predicate is truthy', async () => {
219+
const next = vi.fn()
220+
221+
await iffElse(true, hookFcnSync, hookFcnAsync)(hook, next)
222+
223+
assert.equal(hookFcnSyncCalls, 1)
224+
assert.equal(hookFcnAsyncCalls, 0)
225+
expect(next).toHaveBeenCalledOnce()
226+
})
227+
228+
it('runs falseHooks and then next() when predicate is falsy', async () => {
229+
const next = vi.fn()
230+
231+
await iffElse(false, hookFcnSync, hookFcnAsync)(hook, next)
232+
233+
assert.equal(hookFcnSyncCalls, 0)
234+
assert.equal(hookFcnAsyncCalls, 1)
235+
expect(next).toHaveBeenCalledOnce()
236+
})
237+
238+
it('calls next() even when no hooks are provided for the branch', async () => {
239+
const next = vi.fn()
240+
241+
await iffElse(true, undefined, undefined)(hook, next)
242+
243+
expect(next).toHaveBeenCalledOnce()
244+
})
245+
246+
it('awaits async predicate then runs trueHooks and next()', async () => {
247+
const next = vi.fn()
248+
const asyncTrue = () => Promise.resolve(true)
249+
250+
await iffElse(asyncTrue, hookFcnSync, hookFcnAsync)(hook, next)
251+
252+
assert.equal(hookFcnSyncCalls, 1)
253+
assert.equal(hookFcnAsyncCalls, 0)
254+
expect(next).toHaveBeenCalledOnce()
255+
})
256+
257+
it('calls next() after inner hooks finish, not before', async () => {
258+
const order: string[] = []
259+
const slowHook = async (h: any) => {
260+
await new Promise((r) => setTimeout(r, 5))
261+
order.push('inner')
262+
return h
263+
}
264+
const next = vi.fn(async () => {
265+
order.push('next')
266+
})
267+
268+
await iffElse(true, slowHook, undefined)(hook, next)
269+
270+
assert.deepEqual(order, ['inner', 'next'])
217271
})
218272
})
219273
})

‎src/hooks/iff-else/iff-else.hook.ts‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import type { HookContext } from '@feathersjs/feathers'
1+
import type { HookContext, NextFunction } from '@feathersjs/feathers'
22
import { isPromise } from '../../common/index.js'
33
import { combine } from '../../utils/combine/combine.util.js'
44
import type { HookFunction, PredicateFn } from '../../types.js'
@@ -27,7 +27,7 @@ export function iffElse<H extends HookContext = HookContext>(
2727
falseHook?: HookFunction<H> | HookFunction<H>[] | undefined,
2828
) {
2929
// fnArgs is [context] for service & permission hooks, [data, connection, context] for event filters
30-
return function (this: any, ctx: H) {
30+
return function (this: any, ctx: H, next?: NextFunction) {
3131
const trueHooks = Array.isArray(trueHook)
3232
? trueHook
3333
: typeof trueHook === 'function'
@@ -47,17 +47,18 @@ export function iffElse<H extends HookContext = HookContext>(
4747
? predicate.apply(that, [ctx])
4848
: !!predicate
4949

50-
if (!check) {
51-
return callHooks.call(that, ctx, falseHooks as any)
52-
}
53-
5450
if (!isPromise(check)) {
55-
return callHooks.call(that, ctx, trueHooks as any)
51+
return callHooks.call(
52+
that,
53+
ctx,
54+
(check ? trueHooks : falseHooks) as any,
55+
next,
56+
)
5657
}
5758

5859
return check.then((check1: any) => {
5960
const hooks = check1 ? trueHooks : falseHooks
60-
return callHooks.call(that, ctx, hooks as any)
61+
return callHooks.call(that, ctx, hooks as any, next)
6162
})
6263
}
6364
}
@@ -66,6 +67,16 @@ function callHooks<H extends HookContext = HookContext>(
6667
this: any,
6768
ctx: H,
6869
serviceHooks: HookFunction<H>[],
70+
next?: NextFunction,
6971
) {
70-
return serviceHooks ? combine(...serviceHooks).call(this, ctx) : ctx
72+
if (!serviceHooks) {
73+
if (next) return next()
74+
return ctx
75+
}
76+
77+
const result = combine(...serviceHooks).call(this, ctx)
78+
79+
if (!next) return result
80+
81+
return Promise.resolve(result).then(() => next())
7182
}

0 commit comments

Comments
 (0)