-
Notifications
You must be signed in to change notification settings - Fork 12
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
Collect context #12
Collect context #12
Conversation
The context updaters are all called before the first middleware: the params are no longer lost and we are no longer obliged to redeclare them at each level of hooks. All middleware is grouped into a single chain and they are then executed around the deepest original method. |
Just one question, after that when you think it's good to go we can publish. |
Oh also, is there a test that makes sure that const sayHi = hooks(name => `Hi ${name}`, [
async (context, next) => {
console.log('context1');
await next();
}
]);
const exclamation = hooks(sayHi, [
async (context, next) => {
await next();
context.result += '!';
}
]); Would now print |
It's already tested by existing tests, I just checked with your code and it works fine. This is why there is this line: |
I don't know if it's related but I have an issue with that: class Service extends FeathersKnex {}
hooks(Service.prototype, [/* called */]);
hooks(Service.prototype, {
method: [/* called */]
});
const service = new Service();
hooks(service, [/* NEVER called */]); // ISSUE HERE !
hooks(service, {
method: [/* called */]
});
await service.method(); EDIT: function walkProtoChain(obj, walker = Object.getOwnPropertyNames) {
const proto = Object.getPrototypeOf(obj);
const inherited = proto !== Object.prototype ? walkProtoChain(proto, walker) : [];
return [...new Set(walker(obj).concat(inherited))];
}
// Before
for (const prop of walkProtoChain(service)) {
module.exports[prop] = service[prop];
}
// After
for (const prop of walkProtoChain(service)) {
module.exports[prop] = typeof service[prop] === 'function'
? service[prop].bind(service)
: service[prop];
} |
I added the possibility to give default values to // withParams(...params: Array<string | [string, any]>)
withParams('id', 'name', 'params')
withParams('id', 'name', ['params', {}]) it('creates context with default params', async () => {
const fn = hooks(hello, {
middleware: [
async (ctx, next) => {
assert.equal(ctx.name, 'Dave');
assert.deepEqual(ctx.params, {});
ctx.name = 'Changed';
await next();
}
],
context: withParams('name', ['params', {}])
});
assert.equal(await fn('Dave'), 'Hello Changed');
}); |
I don't see anything to add, you can merge as soon as you want. |
I've been debating if there is a way to improve the syntax for setting params and other things by allowing to chain it: const sayHi = hooks(name => `Hi ${name}`, [ hook1 ]).params('name').defaults({
name: 'Dave'
}); This would still work on objects: const o = {
sayHi(name) { return `Hi ${name}` }
}
hooks(o, {
sayHi: hooks([ hook1 ]).params('name').defaults({
name: 'Dave'
})
}); |
params.forEach((name, index) => { | ||
context[name] = args[index]; | ||
export function withParams<T = any> (...params: Array<string | [string, any]>) { | ||
return (self: any, _fn: any, args: any[], context: HookContext<T>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can change the signature here but this does need to be documented.
Thank you! I published these changes as v0.3.0. Even though this is backwards compatible, we still need some documentation for it though, otherwise nobody will know about it 🤔 |
Missing on doc:
The problem is my ability to write correct explanatory text in English... |
This PR allows adding several context updaters.
It also adds the withDefaults and withProps methods.