Skip to content

Commit

Permalink
feat(hooks): Performance improvement (#106)
Browse files Browse the repository at this point in the history
  • Loading branch information
daffl committed Feb 7, 2023
1 parent 2490de6 commit c14d07c
Show file tree
Hide file tree
Showing 6 changed files with 126 additions and 158 deletions.
10 changes: 4 additions & 6 deletions main/hooks/src/base.ts
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -23,9 +23,9 @@ export interface HookContext<T = any, C = any> extends BaseHookContext<C> {
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,
Expand Down Expand Up @@ -136,8 +136,6 @@ export class HookManager {
const ContextClass = class ContextClass extends Base {
constructor(data: any) {
super(data);

copyToSelf(this);
}
};
const params = this.getParams();
Expand Down
40 changes: 5 additions & 35 deletions main/hooks/src/utils.ts
Original file line number Diff line number Diff line change
@@ -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<F>(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);
}
Expand Down
6 changes: 4 additions & 2 deletions main/hooks/test/benchmark.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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'));
Expand Down
31 changes: 9 additions & 22 deletions main/hooks/test/class.test.ts
Original file line number Diff line number Diff line change
@@ -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<string>;
Expand All @@ -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<any>;

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();

Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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([
Expand Down
84 changes: 45 additions & 39 deletions main/hooks/test/function.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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');
Expand All @@ -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');
Expand Down Expand Up @@ -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'));
});
Expand All @@ -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');
Expand Down
Loading

0 comments on commit c14d07c

Please sign in to comment.