improve hooks.validate by Uzlopak · Pull Request #4804 · fastify/fastify

Uzlopak · 2023-06-09T00:43:16Z

I want to make the Hooks Class capable of handling other events too. To make it possible to add in future more events, the validation function will check if a field is an Array. Also making hooks extend from NullObject, so prototype pollution is not possible.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

climba03003 · 2023-06-09T06:05:37Z

lib/hooks.js

 Hooks.prototype.validate = function (hook, fn) {
  if (typeof hook !== 'string') throw new FST_ERR_HOOK_INVALID_TYPE()
-  if (supportedHooks.indexOf(hook) === -1) {
+  if (Array.isArray(this[hook]) === false) {


Too relax for the check. I would put the supported hook as the static property of Hooks class.
So it own Hooks.ApplicationHooks, Hooks.LifeCycleHooks and Hooks.CustomHooks.

Then, it should be extendible in future by adding custom hook name to the Hooks.CustomHooks

mcollina

LGTM with @climba03003 considerations

Uzlopak · 2023-06-09T12:10:30Z

I personally disagree. By changing the prototype to NullObject, we only have add and validate and the rest are the arrays of the hooks.

Uzlopak · 2023-07-08T08:21:27Z

I quite dont get, why @climba03003 and @mcollina want to separate the hook names into two or three Objects/Arrays.

In my PR I basically make Hooks inheriting from null property. So Hooks only has the methods validate and add as non-Arrays. The rest are the hookFn-Arrays. So checking if a hookName is a attribute of Hooks and is an Array is sufficient.

@climba03003 wrote that the check is too loose (which is disagree). And that he wants three additional attributes: ApplicationHooks, LifeCycleHooks and CustomHooks.

It makes for me not sense to check three different Objects or Arrays to determine if we have those Hooks already or not, if fastify is just having a method called addHook. If we would have methods like addLifecycleHook or addApplicationHook it would make sense to differentiate them.

Eomm

Can't see the pro on splitting the hooks too right now

If we want to add a CustomHooks, the split should stay in that PR, IMHO

For sure a static array would be helpful if we expose it. I listed the hooks in different plugins tho

lib/hooks.js

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

mcollina · 2023-07-08T11:35:14Z

I quite dont get, why @climba03003 and @mcollina want to separate the hook names into two or three Objects/Arrays.

Can you clarify this? I'm not sure what you refer to.

Uzlopak · 2023-07-08T12:28:13Z

You wrote LGTM with @climba03003 considerations. So I assume you want the changes proposed by @climba03003 ?

I see there is other place using Array.isArray for checking the same thing. So, not blocking.

mcollina · 2023-07-09T13:13:39Z

My concern of the direction of this work is that it might slow us down without us realizing it. I wrote a good part of the hook system to maximize its performance, which meant duplicating some code. I would prefer to have all custom hooks elsewhere.

github-actions · 2024-07-09T00:23:18Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

improve hooks.validate

b34774a

climba03003 previously requested changes Jun 9, 2023

View reviewed changes

mcollina approved these changes Jun 9, 2023

View reviewed changes

Merge branch 'main' into make-hooks-more-generic-step1

46289fa

Eomm reviewed Jul 8, 2023

View reviewed changes

lib/hooks.js Outdated Show resolved Hide resolved

Eomm added the chore Small changes or internal project maintenance label Jul 8, 2023

Update lib/hooks.js

b496fe2

Co-authored-by: Manuel Spigolon <behemoth89@gmail.com>

Eomm approved these changes Jul 8, 2023

View reviewed changes

metcoder95 approved these changes Jul 9, 2023

View reviewed changes

mcollina merged commit 58dfb00 into main Jul 9, 2023

mcollina deleted the make-hooks-more-generic-step1 branch July 9, 2023 13:13

renovate bot mentioned this pull request Jul 28, 2023

fix(deps): update all non-major dependencies jellydn/nft-app#1018

Merged

1 task

github-actions bot locked as resolved and limited conversation to collaborators Jul 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

improve hooks.validate#4804

improve hooks.validate#4804
mcollina merged 3 commits intomainfrom
make-hooks-more-generic-step1

Uzlopak commented Jun 9, 2023

Uh oh!

climba03003 Jun 9, 2023

Uh oh!

mcollina left a comment

Uh oh!

Uzlopak commented Jun 9, 2023

Uh oh!

Uzlopak commented Jul 8, 2023

Uh oh!

Eomm left a comment

Uh oh!

Uh oh!

mcollina commented Jul 8, 2023

Uh oh!

Uzlopak commented Jul 8, 2023

Uh oh!

mcollina commented Jul 9, 2023

Uh oh!

github-actions bot commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

Uzlopak commented Jun 9, 2023

Checklist

Uh oh!

climba03003 Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Jun 9, 2023

Uh oh!

Uzlopak commented Jul 8, 2023

Uh oh!

Eomm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mcollina commented Jul 8, 2023

Uh oh!

Uzlopak commented Jul 8, 2023

Uh oh!

mcollina commented Jul 9, 2023

Uh oh!

github-actions bot commented Jul 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants