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
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.
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 fordisallow
. I have this now: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:
but that has a few downsides.
Problem
Many hooks in
feathers-hooks-common
are synchronous. Addingasync
to all of them would cause:disallow('external')(context)
. You would have to addawait
everywhere you use these. If you forget oneawait
the code can go crazy.Promise
overheadSolution
Allow
AroundHookFunction
also to be synchronous. See https://github.com/feathersjs/feathers/blob/dove/packages/feathers/src/declarations.ts#L456Would become:
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#L8Conclusion:
I understand the concept of
async first
andasync 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 infeathers-hooks-common
I wanted to hear your opinion.