-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Notes on regular hooks support #97
Comments
These are all valid points that I missed.
|
daffl
added a commit
that referenced
this issue
Jun 4, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1.
fromErrorHooks
vs.map(fromErrorHook)
:Initially there were only
fromErrorHooks
which were backwards compatible with feathers 4 error hooks - they all run in a singlecatch()
call, so if one of them throws too then others do not run. I addedfromErrorHook
as a utility that person can use to get a singlecatch()
hook. So if you do.map(fromErrorHook)
then you get a bunch ofcatch()
calls, not one as before.I'm ok with this breaking change, but I assume that you are not and that it might have been overlooked. (If it was deliberate decision than all is good.)
2. Who should go first
afterHooks
orbeforeHooks
?Small thing, but I think it is better for
beforeHooks
to run beforeafterHooks
since there is no point to call an after hook if a before hook throws, it will just add noise to a trace. (I'm talking about this line.)3.
type
should be required inrunHook
internal util:It was optional for
fromHooks
and required forfromHook
since there are nofromHooks
now,type
should be non-optional argument andif (type)
can be simply removed. (This of course depends on the resolution for first point, iffromHooks
will be back than this one needs no change.)4. Propagation of bad practice with support for returning
{...context}
:As I said in #1443 and #2462 supporting for returning new context object from a regular hook is a wrong choice (no upside beside backwards compatibility for obscure feature and a lot of downside). Now it tries to migrate from
feathers
intohooks
. Additional example for why it is not right way: in a hook manager there isprops()
method with which you can set additional properties on a context, butprops()
does not simplyObject.assign
passed properties, it copies property descriptors, allowing for readonly and getters/setters things. Support for practice of{...context}
goes against this, since all of those descriptors will be lost and will lead to bugs when those props get used before a cloned context is returned. (Thought about it when was looking how to migratecontext.statusCode
intocontext.http.statusCode
with getters/setters.)The text was updated successfully, but these errors were encountered: