Skip to content
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

Incorrect type definition for genReqId in FastifyServerOptions #4748

Closed
2 tasks done
sergburn opened this issue May 12, 2023 · 3 comments · Fixed by #4784
Closed
2 tasks done

Incorrect type definition for genReqId in FastifyServerOptions #4748

sergburn opened this issue May 12, 2023 · 3 comments · Fixed by #4784
Labels
bug Confirmed bug typescript TypeScript related

Comments

@sergburn
Copy link
Contributor

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.17.0

Plugin version

No response

Node.js version

Irrelevant

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

Irrelevant

Description

The type of genReqId server option implies its only argument is an instance of FastifyRequest:

    genReqId?: <...>(req: FastifyRequest<...>) => string,

However, in practice it is given RawRequest (e.g. http.IncomingMessage):

function routeHandler (req, res, params, context, query) {
    const id = genReqId(req)

while FastifyRequest is created only later in this function.

Steps to Reproduce

To reproduce, let's extend the test in test/genReqId.js:

    genReqId: function (req) {
      t.hasProp(req, 'id')
      t.hasProp(req, 'raw')
      return 'a'
    }

This test will now fail, because these two props normally found in FastifyRequest instance do not actually exist. (Well, id value is supposed to be provided by genReqId in the first place)

Expected Behavior

The type definition should be something like this:

genReqId?: <...>(req: RawRequest) => string,
@sergburn sergburn changed the title Incorrect type definition for genReqId in FastifyServerOptions (?) Incorrect type definition for genReqId in FastifyServerOptions May 12, 2023
@mcollina
Copy link
Member

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

@mcollina mcollina added bug Confirmed bug typescript TypeScript related labels May 13, 2023
@sergburn
Copy link
Contributor Author

Yes, I'd be glad to contribute! It may take some time though...

sergburn added a commit to sergburn/fastify that referenced this issue Jun 3, 2023
…ves raw request as argument, not FastifyRequest
@Fdawgs Fdawgs linked a pull request Jun 3, 2023 that will close this issue
4 tasks
@sergburn
Copy link
Contributor Author

sergburn commented Jun 3, 2023

BTW, looks like the definition for genReqId in FastifyLoggerOptions is also incorrect. It mentions RawRequest, but that in fact is extends FastifyRequest, which is weird, or at least not similar to other places where "RawRequest" name is used.

However, it probably doesn't matter, because this genReqId is never called, AFAICS.

climba03003 pushed a commit to sergburn/fastify that referenced this issue Jun 3, 2023
…ves raw request as argument, not FastifyRequest
Uzlopak added a commit that referenced this issue Jun 16, 2023
* Fixes #4748: correct type defiinition for genReqId(), it receives raw request as argument, not FastifyRequest

* Clarify this in docs

* Update docs/Reference/Server.md

Co-authored-by: Frazer Smith <frazer.dev@outlook.com>

* Improved tests

* Removed irrelevant tests

---------

Co-authored-by: Frazer Smith <frazer.dev@outlook.com>
Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug typescript TypeScript related
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants