feathers

feathers copied to clipboard

LTDR: Synchronous hooks in .hooks({ around: .... }) shouldn't throw a ts error.

Motivation:

I'm currently in the process of adding around hooks to feathers-hooks-common. For example for disallow. I have this now:

export const disallow =
  <H extends HookContext = HookContext>(...transports: TransportName[]) =>
  (context: H, next?: NextFunction) => {
    // do stuff

    if (next) {
      return next();
    }

    return context;
  };

So the disallow function returns a synchronous hook function. Adding that to a real service .hooks(...) example looks like this:

It shows an error that the hook function has to be synchronous. The fix would be to write:

export const disallow =
  <H extends HookContext = HookContext>(...transports: TransportName[]) =>
  async (context: H, next?: NextFunction) => {
    // do stuff

    if (next) {
      return next();
    }

    return context;
  };

but that has a few downsides.

Problem

Many hooks in feathers-hooks-common are synchronous. Adding async to all of them would cause:

• Breaking changes that are hard to debug, because people may use hooks as utils like disallow('external')(context). You would have to add await everywhere you use these. If you forget one await the code can go crazy.
• Small performance damages because of the Promise overhead

Solution

Allow AroundHookFunction also to be synchronous. See https://github.com/feathersjs/feathers/blob/dove/packages/feathers/src/declarations.ts#L456

// New @feathersjs/hook typings
export type AroundHookFunction<A = Application, S = Service> = (
  context: HookContext<A, S>,
  next: NextFunction
) => Promise<void>

Would become:

// New @feathersjs/hook typings
export type AroundHookFunction<A = Application, S = Service> = (
  context: HookContext<A, S>,
  next: NextFunction
) => void | Promise<void>

But then this error shows up: see https://github.com/feathersjs/feathers/blob/dove/packages/feathers/src/hooks.ts#L161

So Middleware of @feathersjs/hooks also needs to allow sync hooks: https://github.com/feathersjs/hooks/blob/main/main/hooks/src/compose.ts#L8

Conclusion:

I understand the concept of async first and async only and I highly encourage it where it's reasonable. But at least in these cases it's counter intuitive:

• disablePagination
• disallow
• discardQuery
• discard
• preventChanges
• setField
• setNow

Before I make the Middleware also synchronous, etc. which results in 2 PRs and 2 releases until I can use it in feathers-hooks-common I wanted to hear your opinion.