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.

Sign up for GitHub

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

Jump to bottom

fastify.log.isLevelEnabled() is missing from TypeScript types #5382

Open
2 tasks done
matt-smarsh opened this issue Apr 1, 2024 · 4 comments · May be fixed by #5397
Open
2 tasks done

fastify.log.isLevelEnabled() is missing from TypeScript types #5382

matt-smarsh opened this issue Apr 1, 2024 · 4 comments · May be fixed by #5397

Comments

@matt-smarsh
Copy link

matt-smarsh commented Apr 1, 2024 •
edited

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.26.2

Plugin version

No response

Node.js version

18.19.1

Operating system

Windows

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

10

Description

Pino exposes a function logger.isLevelEnabled() which is available withing Fastify, but the TypeScript typing doesn't implement it. The type FastifyBaseLogger should be updated to extend pino.Logger in order to have the exact interface which matches what pino is exposing.

Conveniently, pino.Logger also implements child() so that can be removed from Fastify's types.

Inside types/logger.d.ts:

export interface FastifyBaseLogger<CustomLevels extends string = never>
  extends pino.Logger<CustomLevels> {}

Steps to Reproduce

With the following TypeScript:

import Fastify from "fastify"

const server = fastify({ logger: true })

console.log(`Trace enabled: `, server.log.isLevelEnabled("trace"))

Expected Behavior

The value of server.log should expose the full Pino interface.
@climba03003
Copy link
Member

climba03003 commented Apr 2, 2024 •
edited

I would say it is a wontfix because we need to support custom logger with minimal pino interface.
Chasing it back and forth to provide complete pino API and compatible minimal interface by default is not realistic currently.

As a workaround, you can supply the pino interface to generic argument of TypeScript.

cc @fastify/typescript

@matt-smarsh
Copy link
Author

That makes sense. Supporting custom loggers wasn't on my mind but perhaps it requires some more thought...

The generic could extend the BaseLogger, but default to Pino's full logger. That should support the current API contract. I feel like this should still merit consideration as the current TypeScript types are masking existing default functionality.

@climba03003
Copy link
Member

The generic could extend the BaseLogger, but default to Pino's full logger.

That sound do-able. We may try to see how it works.

@matt-smarsh matt-smarsh linked a pull request Apr 9, 2024 that will close this issue
Open
4 tasks
@matt-smarsh
Copy link
Author

I took a stab at it in the PR above ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

types: Use full pino API as Logger default matt-smarsh/fastify
2 participants
@climba03003 @matt-smarsh