fix(async-hooks): mixing async and callback style in preHandler option now returns an error by giuliowaitforitdavide · Pull Request #5069 · fastify/fastify

giuliowaitforitdavide · 2023-10-02T11:06:14Z

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

…n now returns an error

giuliowaitforitdavide · 2023-10-02T11:10:52Z

@Eomm the main thing I did was to add this check besides the already present checks. What do you think about adding them all inside the route.js too? Would it be more complete and less error prone?

mcollina

lgtm

Eomm · 2023-10-02T11:52:20Z

                throw new FST_ERR_HOOK_INVALID_HANDLER(hook, Object.prototype.toString.call(func))
              }
+
+              if (func.constructor.name === 'AsyncFunction' && func.length === 3) {


not all hooks have the same parameters:

'onTimeout' - 3
'onRequest' - 3
'preParsing' - 4
'preValidation' - 3
'preSerialization' - 4
'preHandler' - 3
'onSend' - 4
'onResponse' - 3
'onError' - 4
'onRequestAbort - 2

Following your suggestion and the check done here I came up with a small refactor. Do you think it's worth it or should I revert it?

it does make sense 👍🏼
but I think we should not rewrite too much the code as the additional functions and datastructure may slowdown the start time

@Eomm

…hanged following already present validation and @Eomm suggestions

Co-authored-by: James Sumners <321201+jsumners@users.noreply.github.com>

giuliowaitforitdavide · 2023-10-02T14:08:51Z

I need your help: I don't understand why the pipeline is failing

jsumners · 2023-10-02T14:29:57Z

I need your help: I don't understand why the pipeline is failing

Review https://github.com/fastify/fastify/actions/runs/6381041796/job/17316704069?pr=5069

Eomm · 2023-10-02T16:28:23Z

    })
 }

+function validateAsyncHooks (name, fn) {


I would keep the old code.
It may seem ugly, but it is fast and with a low memory footprint

https://backend.cafe/the-secrets-behind-high-performance-with-nodejs#heading-silly-code-is-not-so-silly

This reverts commit 2ae3fd3.

This reverts commit c6e3888.

@Eomm

…ce and changed following already present validation and @Eomm suggestions" This reverts commit 4a62008.

…structures

mcollina · 2023-10-03T12:29:42Z

cc @Eomm

Uzlopak · 2023-10-03T12:35:28Z

We actually should consider exporting the information regarding the available hooks(preSerializer, onReady...), their type(application hook or LifecycleHook) and their argument count, and maybe if they have payload.

We had so many times the issue, that we implemented in plugins code which needed this info.

Eomm · 2023-10-03T14:59:22Z

We had so many times the issue, that we implemented in plugins code which needed this info.

I'm still PRO to this - will open a issue for that

github-actions · 2024-10-04T00:27:05Z

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.

fix(async-hooks): mixing async and callback style in preHandler optio…

1bba51f

…n now returns an error

mcollina approved these changes Oct 2, 2023

View reviewed changes

Eomm requested changes Oct 2, 2023

View reviewed changes

refactor(async-hooks-validation): moved validation in one place and c…

4a62008

…hanged following already present validation and @Eomm suggestions

jsumners reviewed Oct 2, 2023

View reviewed changes

Comment thread lib/validation.js Outdated

giuliowaitforitdavide added 3 commits October 2, 2023 15:09

style(hooks-validation): removed negated false

c6e3888

348dae3

28c0fac

jsumners reviewed Oct 2, 2023

View reviewed changes

Comment thread lib/validation.js Outdated

Update lib/validation.js

2ae3fd3

Co-authored-by: James Sumners <321201+jsumners@users.noreply.github.com>

Eomm reviewed Oct 2, 2023

View reviewed changes

Eomm added the bugfix Issue or PR that should land as semver patch label Oct 2, 2023

giuliowaitforitdavide added 5 commits October 2, 2023 20:16

Revert "Update lib/validation.js"

1a72522

This reverts commit 2ae3fd3.

Revert "style(hooks-validation): removed negated false"

7de902a

This reverts commit c6e3888.

Revert "refactor(async-hooks-validation): moved validation in one pla…

3fe7880

…ce and changed following already present validation and @Eomm suggestions" This reverts commit 4a62008.

perf(async-hook-validation): improved performance with simplier data …

0f0a896

…structures

test(hooks-async-validation): improved coverage to 100%

51bc299

metcoder95 approved these changes Oct 2, 2023

View reviewed changes

jsumners approved these changes Oct 3, 2023

View reviewed changes

Eomm approved these changes Oct 3, 2023

View reviewed changes

Eomm merged commit 16859bb into fastify:main Oct 3, 2023

giuliowaitforitdavide deleted the fix/#5068 branch October 3, 2023 15:07

github-actions Bot locked as resolved and limited conversation to collaborators Oct 4, 2024

Uh oh!

Conversation

giuliowaitforitdavide commented Oct 2, 2023

Checklist

Uh oh!

giuliowaitforitdavide commented Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Eomm Oct 2, 2023

Choose a reason for hiding this comment

Uh oh!

giuliowaitforitdavide Oct 2, 2023

Choose a reason for hiding this comment

Uh oh!

Eomm Oct 2, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

giuliowaitforitdavide commented Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsumners commented Oct 2, 2023

Uh oh!

Eomm Oct 2, 2023

Choose a reason for hiding this comment

Uh oh!

mcollina commented Oct 3, 2023

Uh oh!

Uzlopak commented Oct 3, 2023

Uh oh!

Eomm commented Oct 3, 2023

Uh oh!

github-actions Bot commented Oct 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

giuliowaitforitdavide commented Oct 2, 2023 •

edited

Loading

giuliowaitforitdavide commented Oct 2, 2023 •

edited

Loading