fix(core): context.type for around hooks

[fratzinger]

Current implementation is null for 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:

context.type is a read only property with the hook type (one of around, before, after or error).

https://dove.feathersjs.com/api/hooks.html#context-type

[netlify]

✅ Deploy Preview for feathers-dove ready!

Name	Link
🔨 Latest commit	9720b12
🔍 Latest deploy log	https://app.netlify.com/sites/feathers-dove/deploys/63937078bad4ea00084c7b74
😎 Deploy Preview	https://deploy-preview-2890--feathers-dove.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[daffl]

This makes sense. I don't remember if this is actually the case now. Could you confirm if context.type ==== 'around' and not null in an around hook? Also, merging with latest dove should fix the broken test.

[fratzinger]

No, it's context.type ==== null currently. But I have absolutely no idea how to change that. I looked back and forth but it's too hidden for me. Can you give me a pointer?

[daffl]

I'm wondering if for performance reasons we should document it as being null for around hooks. The type assignment is happening in https://github.com/feathersjs/hooks/blob/main/main/hooks/src/regular.ts which doesn't have a concept or around hooks. Another option would be to add a hook that sets the type in https://github.com/feathersjs/feathers/blob/dove/packages/feathers/src/hooks.ts#L61

[fratzinger]

For performance reasons? I'm irritated.

https://github.com/feathersjs/feathers/blob/dove/packages/feathers/src/hooks.ts#L175 setting type: 'around' should do the trick, shouldn't it? I'll give it a try. Although not very clean. But this hooks.ts file is pretty hard to read already.

• enable vs. collect vs. register.
• createContext vs. initializeContext
• manager / manager.params().props() / manager.middleware()

[daffl]

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 context.type in hook2 to null.

[fratzinger]

I understood that @feathersjs/hooks is a general purpose library that can be used for every possible function.

Why do we have this feathers-specific type assignment then over there?:

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 type: 'around'
and https://github.com/feathersjs/hooks/blob/main/main/hooks/src/regular.ts#L17

if (type) context.type = 'around';

should be a quick fix.

[daffl]

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.

[fratzinger]

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 regular.ts file of course has to live there.

I thought the @feathersjs/hooks package could be used outside of the feathers world with various formats of context. Possibly with a context that does not have a context.type prop at all.

Therefore I only complain about the context.type = ... assignment because it's specific to feathers. And because it's feathers specific, it should somehow live in the feathersjs/feathers repo somehow.

The abstraction of @feathersjs/hooks is nice but the separation needs a little bit more fine-tuning.

[daffl]

Agreed on the separation. The context of @feathersjs/hooks already has some additional properties (like method, result and arguments - see https://github.com/feathersjs/hooks#hook-context) so adding a type (which it already does for the regular wrapper) wouldn't be against the design though.

[fratzinger]

Please have a look at feathersjs/hooks#104.

In conjunction with https://github.com/feathersjs/feathers/blob/dove/packages/feathers/src/hooks.ts#L175 type: 'around' this should probably work.