diff --git a/.claude/rules/preserve-type-safety-during-refactoring.md b/.claude/rules/preserve-type-safety-during-refactoring.md new file mode 100644 index 0000000..6b43369 --- /dev/null +++ b/.claude/rules/preserve-type-safety-during-refactoring.md @@ -0,0 +1,55 @@ +--- +title: Preserve Type Safety During Refactoring +impact: MEDIUM +paths: + - "**/*.ts" +--- + +# Preserve Type Safety During Refactoring + +Count `as` type assertions before and after any refactoring. The refactored code must have equal or fewer assertions than the original. Increasing assertion count indicates the new abstractions fight the type system rather than working with it. + +## Incorrect + +Merging two typed functions into one with an `unknown` parameter, requiring new `as` casts at every usage site. + +```typescript +// Original: 2 functions, 0 extra assertions +const handleSuccess = (result: R, hooks: Hooks): R => { + return hooks.onReturn ? hooks.onReturn(result) : result; +}; +const handleError = (error: unknown, hooks: Hooks): R => { + if (hooks.onError) return hooks.onError(error); + throw error; +}; + +// Refactored: 1 function, but 3 new assertions +const settle = (succeeded: boolean, value: unknown, hooks: Hooks): R => { + if (succeeded) { + return hooks.onReturn + ? (hooks.onReturn(value as R) as R) // 2 new assertions + : (value as R); // 1 new assertion + } + // ... +}; +``` + +## Correct + +Use overloads or a discriminated union to preserve type information without additional casts. + +```typescript +type Outcome = + | { succeeded: true; value: R } + | { succeeded: false; error: unknown }; + +const settle = (outcome: Outcome, hooks: Hooks): R => { + if (outcome.succeeded) { + return hooks.onReturn + ? hooks.onReturn(outcome.value) // no cast needed, value is R + : outcome.value; + } + if (hooks.onError) return hooks.onError(outcome.error); + throw outcome.error; +}; +``` diff --git a/README.md b/README.md index f5f85bc..a070729 100644 --- a/README.md +++ b/README.md @@ -147,6 +147,64 @@ class Service { } ``` +### Async hooks + +When the decorated method returns a `Promise`, all hooks may optionally return a `Promise` as well. `onReturn` receives the **unwrapped** resolved value, and the library automatically chains the returned promise so async hooks execute in the correct order. + +```typescript +import { Effect } from 'base-decorators'; + +const DelayedLog = () => Effect({ + // Returning a Promise delays the original method + onInvoke: async () => { + await new Promise((resolve) => setTimeout(resolve, 10)); + console.log('starting...'); + }, + + // `result` is the unwrapped string, not Promise + onReturn: async ({ result }) => { + console.log('done:', result); + return `${result} (modified)`; + }, +}); + +class Service { + @DelayedLog() + async greet(name: string): Promise { + return `Hello, ${name}`; + } +} + +const service = new Service(); +const value = await service.greet('World'); +// "Hello, World (modified)" +``` + +Async `onError` and `finally` hooks work the same way. Here, an async `finally` flushes a log buffer after every call: + +```typescript +import { Effect } from 'base-decorators'; + +const buffer: string[] = []; + +const FlushAfterCall = () => Effect({ + onInvoke: ({ args }) => { + buffer.push('invoke: ' + String(args)); + }, + finally: async () => { + await fetch('/log', { method: 'POST', body: JSON.stringify(buffer) }); + buffer.length = 0; + }, +}); + +class Worker { + @FlushAfterCall() + async doWork(id: number): Promise { + // async work + } +} +``` + ### Class and Method decorators `Effect` and all hook decorators can be used on both classes and methods out of the box. diff --git a/src/effect-on-method.ts b/src/effect-on-method.ts index f1479b5..bbeb3d1 100644 --- a/src/effect-on-method.ts +++ b/src/effect-on-method.ts @@ -1,5 +1,5 @@ import { setMeta, SYM_META_PROP } from './set-meta.decorator'; -import type { EffectHooks, HookContext, HooksOrFactory } from './hook.types'; +import type { EffectHooks, HookContext, HooksOrFactory, UnwrapPromise } from './hook.types'; import { getParameterNames } from './getParameterNames'; /** @@ -59,40 +59,13 @@ export const EffectOnMethod = ( // Extract parameter names at decoration time (once, not per-call) const parameterNames = getParameterNames(originalMethod); - const wrapped = function (this: object, ...args: unknown[]): unknown { - const argsObject = buildArgsObject(parameterNames, args); - const className = (this.constructor as { name: string }).name ?? ''; - - const context: HookContext = { - argsObject, - args, - target: this, - propertyKey, - descriptor, - parameterNames, - className, - }; - - const hooks = resolveHooks(hooksOrFactory, context); - - if (hooks.onInvoke) { - hooks.onInvoke(context); - } - - try { - const result = originalMethod.apply(this, args); - - if (result instanceof Promise) { - return chainAsyncHooks(result as Promise, context, hooks); - } - - // If an error occurs in the onReturn hook, `finally` will be triggered 2 times. - // Intentional architecture choice due to TS limitations - return handleSyncSuccess(result as R, context, hooks); - } catch (error: unknown) { - return handleSyncError(error, context, hooks); - } - }; + const wrapped = wrapFunction( + originalMethod, + parameterNames, + propertyKey, + descriptor, + hooksOrFactory, + ); copySymMeta(originalMethod, wrapped); @@ -104,6 +77,8 @@ export const EffectOnMethod = ( }; }; + + /** * Builds an object mapping parameter names to their values. * @@ -112,7 +87,7 @@ export const EffectOnMethod = ( * * @param parameterNames - Array of parameter names * @param args - Array of argument values - * @returns Object mapping parameter names to values + * @returns Object mapping parameter names to values, or undefined when empty * * @example * buildArgsObject(['id', 'name'], [1, 'John']) @@ -120,7 +95,10 @@ export const EffectOnMethod = ( * * @internal */ -export const buildArgsObject = (parameterNames: string[], args: unknown[]): Record | undefined => { +export const buildArgsObject = ( + parameterNames: string[], + args: unknown[], +): Record | undefined => { if (args.length === 0 && parameterNames.length === 0) { return undefined; } @@ -136,6 +114,89 @@ export const buildArgsObject = (parameterNames: string[], args: unknown[]): Reco return argsObject; }; +/** + * Builds the per-method wrapper used by {@link EffectOnMethod}: constructs + * {@link HookContext}, resolves hooks, and wires `onInvoke` plus execution. + */ +export const wrapFunction = ( + originalMethod: (...args: unknown[]) => unknown, + parameterNames: string[], + propertyKey: string | symbol, + descriptor: PropertyDescriptor, + hooksOrFactory: HooksOrFactory, +): ((this: object, ...args: unknown[]) => unknown) => + function (this: object, ...args: unknown[]): unknown { + const argsObject = buildArgsObject(parameterNames, args); + const className = (this.constructor as { name: string }).name ?? ''; + + const context: HookContext = { + argsObject, + args, + target: this, + propertyKey, + descriptor, + parameterNames, + className, + }; + + const hooks = resolveHooks(hooksOrFactory, context); + + const executeMethod = attachHooks(originalMethod, this, args, context, hooks); + + if (hooks.onInvoke) { + const invokeResult = hooks.onInvoke(context); + + if (invokeResult instanceof Promise) { + return invokeResult.then(executeMethod); + } + } + + return executeMethod(); + }; + +/** + * Returns a thunk that runs the original method and applies sync/async lifecycle hooks. + * + * Kept as a thunk so async `onInvoke` can defer execution via `.then()`. + * `finally` is applied inline on sync paths to avoid double-calling when + * `onReturn` or `onError` throw. + */ +export const attachHooks = ( + originalMethod: (...args: unknown[]) => unknown, + thisArg: object, + args: unknown[], + context: HookContext, + hooks: EffectHooks, +): (() => unknown) => () => { + try { + const result = originalMethod.apply(thisArg, args); + + if (result instanceof Promise) { + return chainAsyncHooks(result, context, hooks); + } + + try { + return hooks.onReturn + ? hooks.onReturn({ ...context, result: result as UnwrapPromise }) + : result; + } finally { + hooks.finally?.(context); + } + } catch (error: unknown) { + try { + if (hooks.onError) { + return hooks.onError({ ...context, error }); + } + + throw error; + } finally { + hooks.finally?.(context); + } + } +}; + + + /** * Copies the `_symMeta` Map from the original function to a new function. * @@ -144,10 +205,7 @@ export const buildArgsObject = (parameterNames: string[], args: unknown[]): Reco * or `@NoLog`) must survive on the new wrapper so downstream consumers * (like `EffectOnClass`) can still read it. */ -const copySymMeta = ( - source: Function, - target: Function, -): void => { +const copySymMeta = (source: Function, target: Function): void => { const sourceRecord = source as unknown as Record; const sourceMap = sourceRecord[SYM_META_PROP] as | Map @@ -167,9 +225,7 @@ const copySymMeta = ( } const targetMap = targetRecord[SYM_META_PROP] as Map; - sourceMap.forEach((value, key) => { - targetMap.set(key, value); - }); + sourceMap.forEach((value, key) => targetMap.set(key, value)); }; /** @@ -189,80 +245,32 @@ const resolveHooks = ( }; /** - * Handles the synchronous success + finally path. + * Applies lifecycle hooks (onReturn, onError, finally) to an async method result. * - * Separated to keep the main decorator body within line limits. + * Uses async/await with try/catch/finally so that onReturn fires after + * resolution, onError fires after rejection, and finally always fires last. */ -const handleSyncSuccess = ( - result: R, +const chainAsyncHooks = async ( + promise: Promise, context: HookContext, hooks: EffectHooks, -): R => { +): Promise => { try { - return hooks.onReturn - ? hooks.onReturn({ ...context, result }) - : result; - } finally { - if (hooks.finally) { - hooks.finally(context); - } - } -}; + const value = await promise; -/** - * Handles the synchronous error + finally path. - * - * Separated to keep the main decorator body within line limits. - */ -const handleSyncError = ( - error: unknown, - context: HookContext, - hooks: EffectHooks, -): R => { - try { + return hooks.onReturn + ? await hooks.onReturn({ ...context, result: value as UnwrapPromise }) + : value; + } catch (error: unknown) { if (hooks.onError) { - return hooks.onError({ ...context, error }); + return await hooks.onError({ ...context, error }); } throw error; } finally { if (hooks.finally) { - hooks.finally(context); + await hooks.finally(context); } } }; -/** - * Chains promise lifecycle hooks for asynchronous method execution. - * - * Uses `.then` / `.catch` / `.finally` on the returned promise so that - * `onReturn` fires after resolution, `onError` fires after rejection, - * and `finally` always fires last. - */ -const chainAsyncHooks = ( - promise: Promise, - context: HookContext, - hooks: EffectHooks, -): Promise => { - let chained = promise; - - if (hooks.onReturn) { - chained = chained.then((value) => { - return hooks.onReturn!({ ...context, result: value }); - }); - } - - if (hooks.onError) { - chained = chained.catch((error: unknown) => { - return hooks.onError!({ ...context, error }); - }); - } - - if (hooks.finally) { - chained = chained.finally(() => { - hooks.finally!(context); - }); - } - - return chained; -}; \ No newline at end of file diff --git a/src/hook.types.ts b/src/hook.types.ts index ef66156..dcb6593 100644 --- a/src/hook.types.ts +++ b/src/hook.types.ts @@ -26,10 +26,16 @@ export interface HookContext { descriptor: PropertyDescriptor; } +/** Extracts the resolved type from a Promise, or returns the type itself. */ +export type UnwrapPromise = T extends Promise ? U : T; + +/** Allows a Promise return only when the original type is already a Promise. */ +export type MaybeAsync = T extends Promise ? U | Promise : T; + /** Context for the onReturn hook, adding the method result. */ export interface OnReturnContext extends HookContext { - /** The value returned by the original method. */ - result: R; + /** The value returned by the original method (unwrapped if it was a Promise). */ + result: UnwrapPromise; } /** Context for the onError hook, adding the thrown error. */ @@ -40,35 +46,39 @@ export interface OnErrorContext extends HookContext { /** * Hook fired before the original method executes. - * @typeParam R - The return type of the decorated method (unused, for consistency) + * @typeParam R - The return type of the decorated method. When it extends Promise, + * the hook may also return a Promise. */ export type OnInvokeHookType = ( context: HookContext, -) => void; +) => R extends Promise ? void | Promise : void; /** * Hook fired after a successful return. Its return value replaces the method result. - * @typeParam R - The return type of the decorated method + * @typeParam R - The return type of the decorated method. When it extends Promise, + * the hook may also return a Promise of the resolved type. */ export type OnReturnHookType = ( context: OnReturnContext, -) => R; +) => MaybeAsync; /** * Hook fired when the method throws. May return a recovery value or re-throw. - * @typeParam R - The return type of the decorated method + * @typeParam R - The return type of the decorated method. When it extends Promise, + * the hook may also return a Promise of the resolved type. */ export type OnErrorHookType = ( context: OnErrorContext, -) => R; +) => MaybeAsync; /** * Hook fired after both success and error paths, regardless of outcome. - * @typeParam R - The return type of the decorated method (unused, for consistency) + * @typeParam R - The return type of the decorated method. When it extends Promise, + * the hook may also return a Promise. */ export type FinallyHookType = ( context: HookContext, -) => void; +) => R extends Promise ? void | Promise : void; /** * Lifecycle hooks for method decoration via Effect-based decorators. diff --git a/tests/EffectOnMethod.spec.ts b/tests/EffectOnMethod.spec.ts index 3b56135..d9ecb74 100644 --- a/tests/EffectOnMethod.spec.ts +++ b/tests/EffectOnMethod.spec.ts @@ -693,6 +693,235 @@ describe('EffectOnMethod', () => { }); }); + describe('async hooks support', () => { + it('should wait for async onInvoke before executing async method', async () => { + const callOrder: string[] = []; + + const hooks: EffectHooks> = { + onInvoke: async () => { + callOrder.push('onInvoke-start'); + await new Promise((resolve) => setTimeout(resolve, 10)); + callOrder.push('onInvoke-end'); + }, + }; + + class TestService { + @EffectOnMethod(hooks) + async greet(name: string) { + callOrder.push('original'); + return `hello ${name}`; + } + } + + const service = new TestService(); + const result = await service.greet('world'); + + expect(result).toBe('hello world'); + expect(callOrder).toEqual(['onInvoke-start', 'onInvoke-end', 'original']); + }); + + it('should wait for async onReturn and replace async method result', async () => { + const hooks: EffectHooks> = { + onReturn: async ({ result }) => { + await new Promise((resolve) => setTimeout(resolve, 10)); + return `${result}-async`; + }, + }; + + class TestService { + @EffectOnMethod(hooks) + async greet(name: string) { + return `hello ${name}`; + } + } + + const service = new TestService(); + const result = await service.greet('world'); + + expect(result).toBe('hello world-async'); + }); + + it('should allow async onError to return recovery value', async () => { + const hooks: EffectHooks> = { + onError: async () => { + await new Promise((resolve) => setTimeout(resolve, 10)); + return 'recovered-async'; + }, + }; + + class TestService { + @EffectOnMethod(hooks) + async failing() { + throw new Error('fail'); + } + } + + const service = new TestService(); + const result = await service.failing(); + + expect(result).toBe('recovered-async'); + }); + + it('should wait for async finally on success', async () => { + const callOrder: string[] = []; + + const hooks: EffectHooks> = { + finally: async () => { + callOrder.push('finally-start'); + await new Promise((resolve) => setTimeout(resolve, 10)); + callOrder.push('finally-end'); + }, + }; + + class TestService { + @EffectOnMethod(hooks) + async greet(name: string) { + callOrder.push('original'); + return `hello ${name}`; + } + } + + const service = new TestService(); + const result = await service.greet('world'); + + expect(result).toBe('hello world'); + expect(callOrder).toEqual(['original', 'finally-start', 'finally-end']); + }); + + it('should wait for async finally on error', async () => { + const callOrder: string[] = []; + + const hooks: EffectHooks> = { + finally: async () => { + callOrder.push('finally-start'); + await new Promise((resolve) => setTimeout(resolve, 10)); + callOrder.push('finally-end'); + }, + }; + + class TestService { + @EffectOnMethod(hooks) + async failing() { + callOrder.push('original'); + throw new Error('fail'); + } + } + + const service = new TestService(); + await expect(service.failing()).rejects.toThrow('fail'); + expect(callOrder).toEqual(['original', 'finally-start', 'finally-end']); + }); + + it('should trigger finally when async onError re-throws on rejection path', async () => { + const callOrder: string[] = []; + const testError = new Error('original error'); + + const hooks: EffectHooks> = { + onError: async ({ error }) => { + callOrder.push('onError-start'); + await new Promise((resolve) => setTimeout(resolve, 10)); + callOrder.push('onError-end'); + throw error; + }, + finally: () => { + callOrder.push('finally'); + }, + }; + + class TestService { + @EffectOnMethod(hooks) + async failing() { + callOrder.push('original'); + throw testError; + } + } + + const service = new TestService(); + await expect(service.failing()).rejects.toThrow(testError); + expect(callOrder).toEqual(['original', 'onError-start', 'onError-end', 'finally']); + }); + + it('should execute all async hooks in correct order', async () => { + const callOrder: string[] = []; + + const hooks: EffectHooks> = { + onInvoke: async () => { + callOrder.push('onInvoke'); + }, + onReturn: async ({ result }) => { + callOrder.push('onReturn'); + return result; + }, + onError: async ({ error }) => { + callOrder.push('onError'); + throw error; + }, + finally: async () => { + callOrder.push('finally'); + }, + }; + + class TestService { + @EffectOnMethod(hooks) + async greet(name: string) { + callOrder.push('original'); + return `hello ${name}`; + } + } + + const service = new TestService(); + await service.greet('world'); + + expect(callOrder).toEqual(['onInvoke', 'original', 'onReturn', 'finally']); + }); + + it('should propagate async onInvoke rejection without calling method', async () => { + const invokeError = new Error('async onInvoke failed'); + const callOrder: string[] = []; + + const hooks: EffectHooks> = { + onInvoke: async () => { + callOrder.push('onInvoke'); + throw invokeError; + }, + }; + + class TestService { + @EffectOnMethod(hooks) + async greet() { + callOrder.push('original'); + return 'hello'; + } + } + + const service = new TestService(); + await expect(service.greet()).rejects.toThrow(invokeError); + expect(callOrder).toEqual(['onInvoke']); + }); + + it('should turn sync method into Promise when onInvoke is async', async () => { + const hooks: EffectHooks = { + onInvoke: async () => { + await new Promise((resolve) => setTimeout(resolve, 10)); + }, + }; + + class TestService { + @EffectOnMethod(hooks) + greet() { + return 'hello'; + } + } + + const service = new TestService(); + const result = service.greet(); + + expect(result).toBeInstanceOf(Promise); + expect(await result).toBe('hello'); + }); + + }); + describe('exclusionKey parameter', () => { it('should mark method with EFFECT_APPLIED_KEY by default', () => { class TestService { diff --git a/tests/effect-on-method-base.spec.ts b/tests/effect-on-method-base.spec.ts new file mode 100644 index 0000000..1af4a5a --- /dev/null +++ b/tests/effect-on-method-base.spec.ts @@ -0,0 +1,151 @@ +import { describe, it, expect, vi } from 'vitest'; + +import { attachHooks, wrapFunction } from '../src/effect-on-method'; +import type { EffectHooks, HookContext, OnReturnContext } from '../src/hook.types'; + +const makeDescriptor = (fn: (...args: unknown[]) => unknown): PropertyDescriptor => ({ + value: fn, + writable: true, + enumerable: true, + configurable: true, +}); + +const makeContext = (overrides: Partial = {}): HookContext => { + const original = () => undefined; + return { + args: [], + argsObject: undefined, + target: {}, + propertyKey: 'method', + parameterNames: [], + className: 'TestCls', + descriptor: makeDescriptor(original), + ...overrides, + }; +}; + +describe('attachHooks', () => { + it('runs the original method and returns its result when hooks are empty', () => { + const original = vi.fn((...args: unknown[]) => (args[0] as number) + 1); + const thisArg = {}; + const args = [2] as unknown[]; + const context = makeContext({ args, target: thisArg }); + + const run = attachHooks(original, thisArg, args, context, {}); + expect(run()).toBe(3); + expect(original).toHaveBeenCalledWith(2); + }); + + it('applies onReturn on sync success', () => { + const original = () => 'a'; + const context = makeContext(); + const run = attachHooks(original, {}, [], context, { + onReturn: ({ result }) => `${result}b`, + }); + expect(run()).toBe('ab'); + }); + + it('applies onError when the original throws', () => { + const original = () => { + throw new Error('fail'); + }; + const context = makeContext(); + const run = attachHooks(original, {}, [], context, { + onError: () => 'recovered', + }); + expect(run()).toBe('recovered'); + }); + + it('calls finally on sync success', () => { + const finallyHook = vi.fn(); + const original = () => 1; + const context = makeContext(); + const run = attachHooks(original, {}, [], context, { finally: finallyHook }); + expect(run()).toBe(1); + expect(finallyHook).toHaveBeenCalledTimes(1); + expect(finallyHook).toHaveBeenCalledWith(context); + }); + + it('resolves async path through chainAsyncHooks', async () => { + const original = () => Promise.resolve(10); + const context = makeContext(); + const run = attachHooks(original, {}, [], context, { + onReturn: ({ result }: OnReturnContext) => result * 2, + }); + await expect(run()).resolves.toBe(20); + }); +}); + +describe('wrapFunction', () => { + it('binds this and args like a decorated method', () => { + class C { + suffix = '!'; + } + const original = function (this: C, name: string) { + return `${name}${this.suffix}`; + }; + const descriptor = makeDescriptor(original as (...args: unknown[]) => unknown); + const wrapped = wrapFunction( + original as (...args: unknown[]) => unknown, + ['name'], + 'greet', + descriptor, + {}, + ); + + const instance = new C(); + expect(wrapped.call(instance, 'hi')).toBe('hi!'); + }); + + it('builds argsObject from parameter names', () => { + let seen: HookContext | undefined; + const original = () => 0; + const descriptor = makeDescriptor(original); + const wrapped = wrapFunction(original, ['a', 'b'], 'm', descriptor, { + onInvoke: (ctx) => { + seen = ctx; + }, + }); + + wrapped.call({}, 1, 2); + expect(seen?.argsObject).toEqual({ a: 1, b: 2 }); + expect(seen?.parameterNames).toEqual(['a', 'b']); + expect(seen?.propertyKey).toBe('m'); + }); + + it('runs async onInvoke then the original', async () => { + const order: string[] = []; + const original = () => { + order.push('original'); + return 1; + }; + const descriptor = makeDescriptor(original); + const wrapped = wrapFunction(original, [], 'm', descriptor, { + onInvoke: async () => { + order.push('onInvoke'); + }, + }); + + await wrapped.call({}); + expect(order).toEqual(['onInvoke', 'original']); + }); + + it('uses hooks factory with per-call context', () => { + const original = (n: number) => n; + const descriptor = makeDescriptor(original as (...args: unknown[]) => unknown); + const factory = vi.fn((_ctx: HookContext): EffectHooks => ({ + onReturn: ({ result }: OnReturnContext) => result + 1, + })); + const wrapped = wrapFunction( + original as (...args: unknown[]) => unknown, + ['n'], + 'm', + descriptor, + factory, + ); + + expect(wrapped.call({}, 5)).toBe(6); + expect(factory).toHaveBeenCalledTimes(1); + expect(factory.mock.calls[0]?.[0]?.args).toEqual([5]); + }); +});