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

add generic logger to route handler & FastifyRequest #3782

Conversation

MarcoLeko
Copy link
Contributor

Checklist

Initiated by this bug: #3775
I created a PR to fix the typings for the routeHandlers by using the FastifyRequest with a generic logger type.
As in the docs mentioned: https://www.fastify.io/docs/latest/Reference/Logging/#logging

You can also supply your own logger instance. Instead of passing configuration options, pass the instance. The logger you supply must conform to the Pino interface; that is, it must have the following methods: info, error, debug, fatal, warn, trace, child.

However with the implemented type definitions it is not possible to use a custom LoggerInstance as the log property was only restricted to be of type FastifyLoggerInstance and no generic.

This is somewhat of an urgent bugfix from our side as it blocks our development and migration from express to fastify. This is also the reason why we set the PR against this branch as we hope to see the fix already in version 3.x. and help other developers as well.

@github-actions github-actions bot added the typescript TypeScript related label Mar 17, 2022
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

cc @kibertoad @fox1t @Eomm could you review?

Copy link
Member

@fox1t fox1t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a non-breaking change. Kudos

test/types/request.test-d.ts Outdated Show resolved Hide resolved
@mcollina mcollina added the backport 3.x Issue or pr that should be backported to Fastify v3 label Mar 17, 2022
@mcollina
Copy link
Member

Can you please keep targeting main? If the change is backportable as-is the backport action would do its job, otherwise you can follow-up with another PR. Thanks.

test/types/request.test-d.ts Outdated Show resolved Hide resolved
@MarcoLeko MarcoLeko changed the base branch from v3.x to main March 18, 2022 10:43
@MarcoLeko MarcoLeko changed the base branch from main to v3.x March 18, 2022 10:44
@MarcoLeko
Copy link
Contributor Author

MarcoLeko commented Mar 18, 2022

Can you please keep targeting main? If the change is backportable as-is the backport action would do its job, otherwise you can follow-up with another PR. Thanks.

Hello @mcollina , since I branched out of 3v.x, I unfurtunately had to deal with huge merge conflicts. Furthermore seems that the FastifyRequest has a lot more extendable properties than in v3.x which effects also several RouteHandlers.

This is why I would recommend to apply these changes here seperately in 3v.x and I could offer you to make a completely different PR in the weekend (this time branching out from main branch) and target the main branch

test/types/request.test-d.ts Outdated Show resolved Hide resolved
@mcollina mcollina removed the backport 3.x Issue or pr that should be backported to Fastify v3 label Mar 18, 2022
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not land on v3.x until there is a matching PR for main.

@mcollina
Copy link
Member

The types in v4 are significantly more enchanced.

@mcollina mcollina added the v3.x Issue or pr related to Fastify v3 label Mar 18, 2022
remove unnecessary type decleration

Co-authored-by: Maksim Sinik <maksim@sinik.it>
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 9738edc into fastify:v3.x Mar 24, 2022
@MarcoLeko MarcoLeko deleted the bugfix/adapt-fastify-request-logger-to-be-generic branch March 24, 2022 12:23
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
typescript TypeScript related v3.x Issue or pr related to Fastify v3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants