feathersjs / feathers

The API and real-time application framework
https://feathersjs.com
MIT License
15.07k stars 751 forks source link

Omitting 'await next()' in around hook gives error: "TypeError: Cannot convert undefined or null to object' #3067

Open tvld opened 1 year ago

tvld commented 1 year ago

In the doc's I did not read anywhere that "doing nothing" in an around hook, will give nasty bug when executing a patch for example: await app.service('users').patch(myId, { name: 'Tom' }).

It happened to me when I had an "if" statement that checked if the hook needed to be executed. But, when execution was not needed, this bug appeared.

Solution 1: clear warning in documentation to always have an await next() in each around hook Solution 2: best: automatically include next() if there is none

To replicate, try this and execute a patch:

//users.hook.ts
export default {
  around: {
    // ...
    patch: [
      authenticate('jwt'),
      async ({ app, params, data, result }: HookContext) => {
        // await next() // Required, omitting will give nasty bug
      },
    ]
  },
daffl commented 1 year ago

There might be a way to throw an error if the parameter length is not as expected but this was also the reason why the new generated application is registering the hooks in the main service file because that ensures type safety. Typing the above would give you a type error:

import { HookMap } from '@feathersjs/feathers'
import { Application } from '../../declarations'
import { MyService } from './myservice'

const hooks: HookMap<Application, MyService> = {
  around: {
    // ...
    patch: [
      authenticate('jwt'),
      async ({ app, params, data, result }: HookContext) => {
        // await next() // Required, omitting will give nasty bug
      },
    ]
  },
}

export default hooks
tvld commented 1 year ago

I tried your solution, but there is still no error warning. What I meant to say is that it is not about the parameter omitting, it is about the await next() omitting in the code, which causes hard-to-understand bugs later on.

daffl commented 1 year ago

Apologies, you're right, it doesn't give a type error which is really strange because it is marked as required in the typings. Unfortunately there isn't a way to determine if and when next has been called at runtime so the approaches I can see here are

tvld commented 1 year ago

All Is it possible to always add another await next() ? Are there downsides of calling await next() twice? In that case it does not matter if you just an around hook as before hook only )