Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(hooks): Performance improvements #106

Merged
merged 1 commit into from
Feb 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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