Skip to content

types: extended FastifyAuthFunction type to accept async functions #260

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ozers
Copy link

@ozers ozers commented May 14, 2025

Checklist

All tests are passing:

  • Unit tests
  • TypeScript tests
  • Benchmark script not included, so I couldn't run.
  • Documentation is not changed because of this change is only affect TypeScript type annotation, we can add if needed.

I would like to open a PR for this issue due to contribute if you allow me.
I've made the the update and updated the tests in this context. Due to TypeScript's inability to automatically infer 'this' context in union types, needed to explicitly specify 'this: FastifyInstance' type annotation in functions that use 'this'. I've updated the tests to reflect this requirement, also added async and promise-based tests to cover the update.

In the function below, which is used without specifying a pre-usable type, the need to use 'this: FastifyInstance' arises due to the situation I mentioned above.

app.auth([
    // this should be transform to function (this: FastifyInstance) for TypeScript annotation needs.
    function () { 
      expectType<FastifyInstance>(this)
      return Promise.resolve()
    },
  ])

Changes made:

  • Updated FastifyAuthFunction type to support both callback and Promise patterns with proper this context
  • Added explicit this type annotation in auth tests
  • Reorganized test structure for better readability
  • tests with { relation: 'or' } added, defaultRelation is already 'or' but defaultRelation definition can be changed.

Please review the changes.

@Uzlopak
Copy link
Contributor

Uzlopak commented May 15, 2025

Afaik this is not working properly. Basically you get a "merged" function. Basically

((this: FastifyInstance, request: Request, reply: Reply, done: (error?: Error) => void) => void)
    | ((this: FastifyInstance, request: Request, reply: Reply) => Promise<void>)

results in

((this: FastifyInstance, request: Request, reply: Reply, done?: (error?: Error) => void) => (void | Promise<void)

compare with
https://github.com/fastify/fastify/blob/be8f72fcb3c99fad1982fe4853119a9a79db40ba/types/hooks.d.ts#L78

@Fdawgs Fdawgs changed the title #258 Extended FastifyAuthFunction type to accept async functions types: extended FastifyAuthFunction type to accept async functions May 15, 2025
@Fdawgs Fdawgs linked an issue May 15, 2025 that may be closed by this pull request
2 tasks
…n, then FastifyAuthFunction updated according to it.
@ozers
Copy link
Author

ozers commented May 15, 2025

Thanks for your comment @Uzlopak , I updated the PR after reviewing the hook link you mentioned and now it looks better and meets the issues mentioned above.

@ozers ozers marked this pull request as ready for review May 15, 2025 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend FastifyAuthFunction type to accept async functions
2 participants