feathersjs / feathers

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

After all and after method specific hook order changed in V5 #3002

Closed KidkArolis closed 1 year ago

KidkArolis commented 1 year ago

Steps to reproduce

If a service has the following hooks:

after: {
  all: [context => console.log('After all')],
  find: [context => console.log('After find')],
}

In V4 it outputs:

After all
After find

In V5 it outputs:

After find
After all

My best current understanding is that it's because of how the before/after/error hooks get collected in a single "collected" chain using the new around hook mechanic here: https://github.com/feathersjs/feathers/blob/5854dea7f610262121a49623ec5bbd474dcd3ef3/packages/feathers/src/hooks.ts#L60-L65

What happens is that the when we spread all and then method specific hooks:

...(collected.all || []),
...(collected[method] || [])

The order for the after hooks becomes:

afterAll() { await next() context => console.log('After all') }

afterFind() { await next() context => console.log('After find') }

and when that unwinds we first yield after find and then after all in that (backwards incompatible) order.

Expected behavior

V5 keeps backwards compatibility in the hooks and logs:

After all
After find

System configuration

Module versions: @feathersjs/feathers@5.0.0-pre.35

KidkArolis commented 1 year ago

Failing test case for feathers/test/hooks/after.test.ts:

it.only('.after all and method specific hooks run in the correct order (#3002)', async () => {
    const app = feathers().use('/dummy', {
      async get(id: any) {
        return { id, items: [] as any }
      }
    })
    const service = app.service('dummy')

    service.hooks({
      after: {
        all(context) {
          context.result.items.push('first')

          return context
        },
        get: [
          function (context) {
            context.result.items.push('second')

            return context
          },
          function (context) {
            context.result.items.push('third')

            return context
          }
        ]
      }
    })

    const data = await service.get(10)

    assert.deepStrictEqual(data.items, ['first', 'second', 'third'])
  })