fix(core): context.type for around hooks#2890
Conversation
✅ Deploy Preview for feathers-dove ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
This makes sense. I don't remember if this is actually the case now. Could you confirm if |
|
No, it's |
|
I'm wondering if for performance reasons we should document it as being |
|
For performance reasons? I'm irritated. https://github.com/feathersjs/feathers/blob/dove/packages/feathers/src/hooks.ts#L175 setting
|
|
I guess the way hooks are collected and run now that should work. The question is just if a registration like service.hooks({
before: { all: [ hook1 ] }
})
service.hooks({
around: { all: [ hook2 ] }
})Would reset |
|
I understood that Why do we have this feathers-specific export const runHook = (hook: RegularMiddleware, context: any, type?: string) => {
if (type) context.type = type;
return Promise.resolve(hook.call(context.self, context))
.then((res: any) => {
if (type) context.type = null;
if (res && res !== context) {
Object.assign(context, res);
}
});
};This is feathers specific and therefore should live in this repo here instead, imo. Anyways:https://github.com/feathersjs/feathers/blob/dove/packages/feathers/src/hooks.ts#L175 setting if (type) context.type = 'around';should be a quick fix. |
|
People were still looking for a general purpose traditional hook wrapper (see https://github.com/feathersjs/hooks#regular-hooks) so @marshallswain voted for having it in there. The terminology has gone through a bit of a process so maybe it should actually be updated in that repository from "async hooks" to "around hooks" as well. |
|
Sorry for the confusion. The general purpose traditional hook wrapper is perfectly valid. The transformation from regular to around hooks is also perfectly fine. That's not my point. The I thought the Therefore I only complain about the The abstraction of |
|
Agreed on the separation. The context of |
|
Please have a look at feathersjs/hooks#104. In conjunction with https://github.com/feathersjs/feathers/blob/dove/packages/feathers/src/hooks.ts#L175 |
contex.type for around hookscontext.type for around hooks
Current implementation is
nullfor around hooks (old name: asynchronous hooks used in jsdoc).I consider this a leftover.
context.type === 'around'is way more intuitive.It's also stated in the docs as this PR fixes it:
https://dove.feathersjs.com/api/hooks.html#context-type