use @feathers/hooks and add async type

[bertho-zero]

This PR uses the new package @feathersjs/hooks and add new type of hook async.

It's fully backward compatible.

NOTE: The mixin in feathers/lib/hooks/index.js is always there only for backward compatibility, so that the additional argument true continues to return the context.

[bertho-zero]

Needs PRs of @feathersjs/hooks

[bertho-zero]

And daffl/uberproto#23, otherwise wild hooks escape!

[daffl]

This looks great and even like we can make it part of a backwards compatible minor release instead of having to wait for the next version. Will work on resolving conflicts and do another review as well as getting out a final @feathersjs/hooks release.

[bertho-zero]

No CODECLIMATE_REPO_TOKEN found. A CODECLIMATE_REPO_TOKEN must be specified as an environment variable.

[KidkArolis]

I've ran this branch through our app's test suite and found 2 changes in behaviour:

1. params is shared between naked service calls

When calling a service without params, the default params are now reused between the calls, but they probably should't be.

// app.hooks.js
module.exports = app => {
  app.hooks({
    before: {
      all: [attachReqId]
    }
  })
}
let reqId = 1
function attachReqId(app) {
  return context => {
    context.params.reqId = context.params.reqId || reqId++
    console.log('Attached reqId', context.params.reqId)
  }
}

// in the test
await service.find()
// -> Attached reqId 1
await service.find()
// -> Attached reqId 1 <--- should be 2, this call is independent from the 1st call and shouldn't share params

2. throwing error without new keyword

I don't know which behaviour is better, but bringing this up as it's inconsistent compared to before.

If a hook throws an error without new, e.g. throw errors.NotFound()

before: service call promise rejects with error
now: service call promise resolves with undefined

[bertho-zero]

Can you share a complete code that shows each case?

For the 1 [attachReqId] should be [attachReqId(app)] in .hook, the context is created when the function is called, so it cannot be shared between calls, normally.

For the 2, all tests passes in https://github.com/feathersjs/feathers/blob/master/packages/feathers/test/hooks/error.test.js and nothing has changed in the @feathersjs/errrors package, that's why I would like a sample code that shows this.

[KidkArolis]

I was trying to put a test case together, but ran into this issue, maybe you can spot the issue / try to run this:

const feathers = require('@feathersjs/feathers')

/* setup */
const app = feathers()
app.configure(services)
app.configure(appHooks)
app.setup()

function appHooks(app) {
  let reqId = 1
  app.hooks({
    before: {
      all: [context => {
        context.params.reqId = context.params.reqId || reqId++
        console.log('Attached reqId', context.params.reqId)
      }]
    }
  })
}

function services(app) {
  app.use('/test', {
    setup() {
      this.counter = 1
    },
    async find(params) {
      return { data: this.counter++ }
    }
  })

  app.service('test').hooks({
    before: {
      all: []
    }
  })
}

/* test */
async function test() {
  console.log('call 1:', await app.service('test').find())
  console.log('call 2:', await app.service('test').find())
}

test()

This works if I install @feathersjs/feathers from npm:

➜  feathers-chat npm i @feathersjs/feathers
➜  feathers-chat node .
Attached reqId 1
call 1: { data: 1 }
Attached reqId 2
call 2: { data: 2 }

But fails if I link in this branch:

➜  feathers-chat npm link @feathersjs/feathers
/Users/karolis/Desktop/feathers-hooks-next/feathers-chat/node_modules/@feathersjs/feathers -> /Users/karolis/.fnm/node-versions/v12.16.1/installation/lib/node_modules/@feathersjs/feathers -> /Users/karolis/Desktop/feathers-hooks-next/feathers/packages/feathers
➜  feathers-chat node .
(node:70911) UnhandledPromiseRejectionWarning: TypeError: finallyWrapper is not a function or its return value is not iterable
    at Function.collect (/Users/karolis/Desktop/feathers-hooks-next/feathers/packages/feathers/lib/hooks/index.js:61:10)
    at /Users/karolis/Desktop/feathers-hooks-next/feathers/packages/feathers/lib/hooks/index.js:45:55
    at Object.wrapper [as _super] (/Users/karolis/Desktop/feathers-hooks-next/feathers/packages/feathers/node_modules/@feathersjs/hooks/lib/function.js:38:16)
    at Object.mixinMethod (/Users/karolis/Desktop/feathers-hooks-next/feathers/packages/feathers/lib/hooks/index.js:97:45)
    at Object.newMethod [as find] (/Users/karolis/Desktop/feathers-hooks-next/feathers/packages/feathers/node_modules/uberproto/lib/proto.js:34:20)
    at test (/Users/karolis/Desktop/feathers-hooks-next/feathers-chat/index.js:40:52)
    at Object.<anonymous> (/Users/karolis/Desktop/feathers-hooks-next/feathers-chat/index.js:44:1)
    at Module._compile (internal/modules/cjs/loader.js:1158:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1178:10)
    at Module.load (internal/modules/cjs/loader.js:1002:32)
(node:70911) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
(node:70911) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

[bertho-zero]

Linking @feathersjs/feathers is not enough, you also need to link @feathersjs/commons.

[KidkArolis]

I linked that too now, same error as above

➜  feathers-chat ls -lah node_modules/@feathersjs
total 0
drwxr-xr-x  4 karolis  staff   128B  5 Mar 13:12 .
drwxr-xr-x  7 karolis  staff   224B  5 Mar 11:55 ..
lrwxr-xr-x  1 karolis  staff    92B  5 Mar 13:12 commons -> ../../../../../.fnm/node-versions/v12.16.1/installation/lib/node_modules/@feathersjs/commons
lrwxr-xr-x  1 karolis  staff    93B  5 Mar 11:56 feathers -> ../../../../../.fnm/node-versions/v12.16.1/installation/lib/node_modules/@feathersjs/feathers

[bertho-zero]

I think you have 2 versions of @feathersjs/commons or @feathersjs/hooks, maybe in the node_modules of @feathersjs/feathers?

[KidkArolis]

Yeah that was it. I ended up copying in the sources into my node_modules. But now I can't reproduce the issue, sorry about wasting time.

I could re-verify if I see these 2 issues in my original project where I ran my test suite, but now I really don't know how to correctly link this in 🤷‍♂️.

[daffl]

Could you resubmit this against https://github.com/feathersjs/feathers/tree/dove? would be good to get in and then I was trying to update it for the hook manager changes (and maybe start the TypeScript refactoring).