From c14d07c0cb4674056c6aadb14dd6f808399a95a3 Mon Sep 17 00:00:00 2001 From: David Luecke Date: Tue, 7 Feb 2023 09:06:54 -0800 Subject: [PATCH] feat(hooks): Performance improvement (#106) --- main/hooks/src/base.ts | 10 ++- main/hooks/src/utils.ts | 40 ++--------- main/hooks/test/benchmark.test.ts | 6 +- main/hooks/test/class.test.ts | 31 +++----- main/hooks/test/function.test.ts | 84 +++++++++++----------- main/hooks/test/object.test.ts | 113 ++++++++++++++++-------------- 6 files changed, 126 insertions(+), 158 deletions(-) diff --git a/main/hooks/src/base.ts b/main/hooks/src/base.ts index 6abfb9c..41a5c6c 100644 --- a/main/hooks/src/base.ts +++ b/main/hooks/src/base.ts @@ -1,5 +1,5 @@ import { AsyncMiddleware } from './compose.ts'; -import { copyProperties, copyToSelf } from './utils.ts'; +import { copyProperties } from './utils.ts'; export const HOOKS: string = Symbol('@feathersjs/hooks') as any; @@ -23,9 +23,9 @@ export interface HookContext extends BaseHookContext { arguments: any[]; } -export type HookContextConstructor = new ( - data?: { [key: string]: any }, -) => BaseHookContext; +export type HookContextConstructor = new (data?: { + [key: string]: any; +}) => BaseHookContext; export type HookDefaultsInitializer = ( self?: any, @@ -136,8 +136,6 @@ export class HookManager { const ContextClass = class ContextClass extends Base { constructor(data: any) { super(data); - - copyToSelf(this); } }; const params = this.getParams(); diff --git a/main/hooks/src/utils.ts b/main/hooks/src/utils.ts index bc3bf87..8d85726 100644 --- a/main/hooks/src/utils.ts +++ b/main/hooks/src/utils.ts @@ -1,45 +1,15 @@ -const proto = Object.prototype as any; -// These are non-standard but offer a more reliable prototype based -// lookup for properties -const hasProtoDefinitions = typeof proto.__lookupGetter__ === 'function' && - typeof proto.__defineGetter__ === 'function' && - typeof proto.__defineSetter__ === 'function'; - -export function copyToSelf(target: any) { - // tslint:disable-next-line - for (const key in target) { - if (!Object.hasOwnProperty.call(target, key)) { - const getter = hasProtoDefinitions - ? target.constructor.prototype.__lookupGetter__(key) - : Object.getOwnPropertyDescriptor(target, key); - - if (hasProtoDefinitions && getter) { - target.__defineGetter__(key, getter); - - const setter = target.constructor.prototype.__lookupSetter__(key); - - if (setter) { - target.__defineSetter__(key, setter); - } - } else if (getter) { - Object.defineProperty(target, key, getter); - } else { - target[key] = target[key]; - } - } - } -} - export function copyProperties(target: F, ...originals: any[]) { for (const original of originals) { - const originalProps = (Object.keys(original) as any) - .concat(Object.getOwnPropertySymbols(original)); + const originalProps = (Object.keys(original) as any).concat( + Object.getOwnPropertySymbols(original), + ); for (const prop of originalProps) { const propDescriptor = Object.getOwnPropertyDescriptor(original, prop); if ( - propDescriptor && !Object.prototype.hasOwnProperty.call(target, prop) + propDescriptor && + !Object.prototype.hasOwnProperty.call(target, prop) ) { Object.defineProperty(target, prop, propDescriptor); } diff --git a/main/hooks/test/benchmark.test.ts b/main/hooks/test/benchmark.test.ts index a29136b..1288920 100644 --- a/main/hooks/test/benchmark.test.ts +++ b/main/hooks/test/benchmark.test.ts @@ -20,7 +20,7 @@ let threshold: number; (async () => { baseline = await getRuntime(() => hello('Dave')); - threshold = baseline * 25; // TODO might be improved further + threshold = baseline * 10; })(); it('empty hook', async () => { @@ -57,7 +57,9 @@ it('single hook, withParams and props', async () => { async (_ctx: HookContext, next: NextFunction) => { await next(); }, - ]).params('name').props({ dave: true }), + ]) + .params('name') + .props({ dave: true }), ); const runtime = await getRuntime(() => hookHello('Dave')); diff --git a/main/hooks/test/class.test.ts b/main/hooks/test/class.test.ts index d0a6105..cdcb68a 100644 --- a/main/hooks/test/class.test.ts +++ b/main/hooks/test/class.test.ts @@ -1,5 +1,5 @@ import { assertEquals, assertStrictEquals, it } from './dependencies.ts'; -import { HookContext, hooks, middleware, NextFunction, WrapperAddon } from '../src/index.ts'; +import { HookContext, hooks, middleware, NextFunction } from '../src/index.ts'; interface Dummy { sayHi(name: string): Promise; @@ -24,17 +24,10 @@ it('hooking object on class adds to the prototype', async () => { hooks(DummyClass, { sayHi: middleware([ async (ctx: HookContext, next: NextFunction) => { - const sayHi = DummyClass.prototype.sayHi as any as WrapperAddon; - - assertEquals( - ctx, - new sayHi.Context({ - arguments: ['David'], - method: 'sayHi', - name: 'David', - self: instance, - }), - ); + assertEquals(ctx.arguments, ['David']); + assertEquals(ctx.method, 'sayHi'); + assertEquals(ctx.name, 'David'); + assertEquals(ctx.self, instance); await next(); @@ -79,14 +72,9 @@ it('works with inheritance', async () => { const DummyClass = createDummyClass(); const first = async (ctx: HookContext, next: NextFunction) => { - assertEquals( - ctx, - new (OtherDummy.prototype.sayHi as any).Context({ - arguments: ['David'], - method: 'sayHi', - self: instance, - }), - ); + assertEquals(ctx.arguments, ['David']); + assertEquals(ctx.method, 'sayHi'); + assertEquals(ctx.self, instance); await next(); @@ -128,8 +116,7 @@ it('works with multiple context updaters', async () => { ]).params('name'), }); - class OtherDummy extends DummyClass { - } + class OtherDummy extends DummyClass {} hooks(OtherDummy, { sayHi: middleware([ diff --git a/main/hooks/test/function.test.ts b/main/hooks/test/function.test.ts index d1c4984..77c67df 100644 --- a/main/hooks/test/function.test.ts +++ b/main/hooks/test/function.test.ts @@ -118,13 +118,7 @@ it('deleting context.result, does not skip method call', async () => { await next(); }; - const fn = hooks( - hello, - middleware([ - updateResult, - deleteResult, - ]), - ); + const fn = hooks(hello, middleware([updateResult, deleteResult])); const res = await fn('There'); assertStrictEquals(res, 'There'); @@ -169,27 +163,33 @@ it('uses hooks from context object and its prototypes', async () => { const o1 = { message: 'Hi' }; const o2 = Object.create(o1); - setMiddleware(o1, [async (ctx: HookContext, next: NextFunction) => { - ctx.arguments[0] += ' o1'; + setMiddleware(o1, [ + async (ctx: HookContext, next: NextFunction) => { + ctx.arguments[0] += ' o1'; - await next(); - }]); + await next(); + }, + ]); - setMiddleware(o2, [async (ctx, next) => { - ctx.arguments[0] += ' o2'; + setMiddleware(o2, [ + async (ctx, next) => { + ctx.arguments[0] += ' o2'; - await next(); - }]); + await next(); + }, + ]); o2.sayHi = hooks( async function (this: any, name: string) { return `${this.message} ${name}`; }, - middleware([async (ctx, next) => { - ctx.arguments[0] += ' fn'; + middleware([ + async (ctx, next) => { + ctx.arguments[0] += ' fn'; - await next(); - }]), + await next(); + }, + ]), ); const res = await o2.sayHi('Dave'); @@ -283,7 +283,9 @@ it('assigns props to context', async () => { await next(); }, - ]).params('name').props({ dev: true }), + ]) + .params('name') + .props({ dev: true }), ); assertStrictEquals(await fn('Dave'), 'Hello Changed'); @@ -292,19 +294,22 @@ it('assigns props to context', async () => { it('assigns props to context by options', async () => { const fn = hooks( hello, - middleware([ - async (ctx, next) => { - assertStrictEquals(ctx.name, 'Dave'); - assertStrictEquals(ctx.dev, true); - - ctx.name = 'Changed'; - - await next(); + middleware( + [ + async (ctx, next) => { + assertStrictEquals(ctx.name, 'Dave'); + assertStrictEquals(ctx.dev, true); + + ctx.name = 'Changed'; + + await next(); + }, + ], + { + params: ['name'], + props: { dev: true }, }, - ], { - params: ['name'], - props: { dev: true }, - }), + ), ); assertStrictEquals(await fn('Dave'), 'Hello Changed'); @@ -443,7 +448,6 @@ it('context has own properties', async () => { assert(keys.includes('self')); assert(keys.includes('message')); - assert(keys.includes('name')); assert(keys.includes('arguments')); assert(keys.includes('result')); }); @@ -468,12 +472,14 @@ it('creates context with default params', async () => { await next(); }, - ]).params('name', 'params').defaults(() => { - return { - name: 'Bertho', - params: {}, - }; - }), + ]) + .params('name', 'params') + .defaults(() => { + return { + name: 'Bertho', + params: {}, + }; + }), ); assertStrictEquals(await fn('Dave'), 'Hello Dave'); diff --git a/main/hooks/test/object.test.ts b/main/hooks/test/object.test.ts index 814e487..5ff4682 100644 --- a/main/hooks/test/object.test.ts +++ b/main/hooks/test/object.test.ts @@ -23,26 +23,24 @@ it('hooks object with hook methods, sets method name', async () => { const obj = getObject(); const hookedObj = hooks(obj, { - sayHi: middleware([async (ctx: HookContext, next: NextFunction) => { - assertEquals(ctx.method, 'sayHi'); - assertEquals( - ctx, - new (obj.sayHi as any).Context({ - arguments: ['David'], - method: 'sayHi', - self: obj, - }), - ); - - await next(); - - ctx.result += '?'; - }]), - addOne: middleware([async (ctx: HookContext, next: NextFunction) => { - ctx.arguments[0] += 1; - - await next(); - }]), + sayHi: middleware([ + async (ctx: HookContext, next: NextFunction) => { + assertEquals(ctx.arguments, ['David']); + assertEquals(ctx.method, 'sayHi'); + assertEquals(ctx.self, obj); + + await next(); + + ctx.result += '?'; + }, + ]), + addOne: middleware([ + async (ctx: HookContext, next: NextFunction) => { + ctx.arguments[0] += 1; + + await next(); + }, + ]), }); assertStrictEquals(obj, hookedObj); @@ -53,29 +51,28 @@ it('hooks object with hook methods, sets method name', async () => { it('hooks object and allows to customize context for method', async () => { const obj = getObject(); const hookedObj = hooks(obj, { - sayHi: middleware([async (ctx: HookContext, next: NextFunction) => { - assertEquals( - ctx, - new (obj.sayHi as any).Context({ - arguments: ['David'], - method: 'sayHi', - name: 'David', - self: obj, - }), - ); - - ctx.name = 'Dave'; + sayHi: middleware([ + async (ctx: HookContext, next: NextFunction) => { + assertEquals(ctx.arguments, ['David']); + assertEquals(ctx.method, 'sayHi'); + assertEquals(ctx.name, 'David'); + assertEquals(ctx.self, obj); - await next(); + ctx.name = 'Dave'; - ctx.result += '?'; - }]).params('name'), + await next(); - addOne: middleware([async (ctx: HookContext, next: NextFunction) => { - ctx.arguments[0] += 1; + ctx.result += '?'; + }, + ]).params('name'), - await next(); - }]), + addOne: middleware([ + async (ctx: HookContext, next: NextFunction) => { + ctx.arguments[0] += 1; + + await next(); + }, + ]), }); assertStrictEquals(obj, hookedObj); @@ -87,19 +84,23 @@ it('hooking multiple times works properly', async () => { const obj = getObject(); hooks(obj, { - sayHi: middleware([async (ctx: HookContext, next: NextFunction) => { - await next(); + sayHi: middleware([ + async (ctx: HookContext, next: NextFunction) => { + await next(); - ctx.result += '?'; - }]), + ctx.result += '?'; + }, + ]), }); hooks(obj, { - sayHi: middleware([async (ctx: HookContext, next: NextFunction) => { - await next(); + sayHi: middleware([ + async (ctx: HookContext, next: NextFunction) => { + await next(); - ctx.result += '!'; - }]), + ctx.result += '!'; + }, + ]), }); assertEquals(await obj.sayHi('David'), 'Hi David!?'); @@ -111,9 +112,11 @@ it('throws an error when hooking invalid method', async () => { assertThrows( () => hooks(obj, { - test: middleware([async (_ctx, next) => { - await next(); - }]), + test: middleware([ + async (_ctx, next) => { + await next(); + }, + ]), }), undefined, `Can not apply hooks. 'test' is not a function`, @@ -132,11 +135,13 @@ it('works with object level hooks', async () => { ]); hooks(obj, { - sayHi: middleware([async (ctx: HookContext, next: NextFunction) => { - await next(); + sayHi: middleware([ + async (ctx: HookContext, next: NextFunction) => { + await next(); - ctx.result += '?'; - }]), + ctx.result += '?'; + }, + ]), }); assertEquals(await obj.sayHi('Dave'), 'Hi Dave?!');