fix: make childLoggerFactory use FastifyBaseLogger

[flakey5]

Fixes #4960

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

[mcollina]

Can you add a test for this? We use tsd.

[flakey5]

Something to note I found when adding the test and have been playing with: pinojs/pino#1858 seems to have made pino's types a bit stricter. pino() used to return Logger<LoggerOptions | DestinationStream> but now the function is generic and returns Logger<never> unless given a generic value (e.g. pino<string>() will return Logger<string>).

This means that pino's Logger type can't be implicitly casted to FastifyBaseLogger anymore. I'm not too sure what would fix this, but wanted to get some input on whether or not this is something y'all would like to be fixed or if this was an intentional change

[mcollina]

What would you recommend we do?

[mcollina]

lgtm

[flakey5]

What would you recommend we do?

I would think trying to make it implicitly castable again makes the most sense, or at least that's what I would expect if I were the user. I'm not entirely sure what that would take though as of now, but I'll look into it and can either add it onto this pr or make another for it

[flakey5]

I would think trying to make it implicitly castable again makes the most sense

From my testing and knowledge I don't think that there's a way we can do this without breaking more user code or removing some type safety.

First thing I tried:

I was able to make it be implicitly castable again by modifying FastifyBaseLogger from this:

export interface FastifyBaseLogger extends pino.BaseLogger {
  child(bindings: Bindings, options? ChildLoggerOptions): FastifyBaseLogger
}

to this:

export interface FastifyBaseLogger<CustomLevels extends string = never> extends pino.BaseLogger, LoggerExtras<CustomLevels> {
                      // also tried defaulting this to pino.Level ^
}

But, this makes FastifyBaseLogger have generics now and that breaks a lot of type tests in the Fastify codebase and I believe types in the users' codebases too from what I can tell. There might be some typescript magic we can do, but, if there is, I'm not aware of it.

Second thing I tried was removing the requirement for the logger to extend FastifyBaseLogger in the first place in FastifyServerOptions. Then in Fastify's internals we can cast it to FastifyBaseLogger where needed. I did this by modifying it from this

export type FastifyServerOptions<
  RawServer extends RawServerBase = RawServerDefault,
  Logger extends FastifyBaseLogger = FastifyBaseLogger
> = {

to this

export type FastifyServerOptions<
  RawServer extends RawServerBase = RawServerDefault,
  Logger = FastifyBaseLogger
> = {

However, I realized there isn't a way I know of to cast the logger to FastifyBaseLogger later on and we'd need to basically allow any object type to be passed in as the Logger all throughout Fastify's types. For example, in order for this code to not error

const instance: FastifyInstance = fastify({ logger: pino() })

I needed to change the generics of FastifyInstance from this

export interface FastifyInstance<
  ...
  Logger extends FastifyBaseLogger = FastifyBaseLogger,
  ...
> {

to this

export interface FastifyInstance<
  ...
  Logger = any,
  ...
> {

In my opinion I think that the second method is the better way to go since this is probably going to land in a minor or patch release and it is the only option I can think of that shouldn't break userland code. However, like I said this does mean there's less type safety with it.

[mcollina]

Go ahead and make that change.

cc @fastify/typescript

[mcollina]

Can you check that req.log.info() will correctly be typed still?

---

I have a rough feeling that the source of the problem is #4150. If we made FastifyBaseLogger not extend pino.BaseLogger. If the two are compatible, I think that could resolve the problem at hand in a better way.

[flakey5]

Can you check that req.log.info() will correctly be typed still?

It is, I'll commit the test I made for that

If we made FastifyBaseLogger not extend pino.BaseLogger.

Just tried and it didn't seem to help

I have a rough feeling that the source of the problem is #4150

I think that's possible but I also think at least part of it might be that it doesn't like that it's trying to implicitly cast a type with generics (pino.Logger) to another type without generics (FastifyBaseLogger). I can't really tell what it doesn't like about the conversion.

I think it might be possible that FastifyBaseLogger is being interpreted as a different type to pino.BaseLogger instead of being treated like the same type (which I think is what's supposed to happen and is what has happened in the past?). That would explain Logger not being castable to it anymore, since it changed from typescript interpreting the type hierarchy from something like this

to something like this

However I have absolutely no idea why this behavior would change so I don't think it's likely to be the reason (unless this has existed even before pino's types got stricter)

[mcollina]

It is, I'll commit the test I made for that

Does it work even if you don't assign any logger?

[Uzlopak]

What if we are appoaching this wrong?
We are strongly coupling the typings of pino with the typings of fastify.
So when you want to use an alternative logger, which uses a pino like api, it is not possible because.

Imho we have to decouple the pino types from the fastify logger types, especially the second parameter of child is using ChildLoggerOptions from the corresponding pino dependency. Actually we somehow have to detect if we actually use pino and then derive the types or if we dont use pino we need to pass an AnyObject '{[key: string]: any}`.

Or we agree on a bare minimum api of the child logger and the logger options.

i have to work, but I thought about something like this. We have to replace then only ChildLoggerOptions and PinoOptions.

Also keep in mind, in v5 we separate logger with loggerConfiguration. So in v5 we could pass a pino like logger, while the loggerConfiguration can and must be a pino configuration.

import { FastifyError } from '@fastify/error'
import { RouteGenericInterface } from './route'
import { FastifyRequest } from './request'
import { FastifyReply } from './reply'
import { RawServerBase, RawServerDefault, RawRequestDefaultExpression, RawReplyDefaultExpression, ContextConfigDefault } from './utils'
import { FastifyTypeProvider, FastifyTypeProviderDefault } from './type-provider'
import { FastifySchema } from './schema'
import { FastifyInstance } from './instance'

import pino from 'pino'

export type PinoLoggerOptions = pino.LoggerOptions
export type ChildLoggerOptions = pino.ChildLoggerOptions

/**
 * Standard Fastify logging function
 */
export type FastifyLogFn = {
  // TODO: why is this different from `obj: object` or `obj: any`?
  /* tslint:disable:no-unnecessary-generics */
  <T extends object>(obj: T, msg?: string, ...args: any[]): void;
  (obj: unknown, msg?: string, ...args: any[]): void;
  (msg: string, ...args: any[]): void;
}

export type LogLevel = 'fatal' | 'error' | 'warn' | 'info' | 'debug' | 'trace' | 'silent'

export type LogLevelOrString = LogLevel | (string & Record<never, never>)

export type Bindings = Record<string, any>

export interface FastifyBaseLogger {
  /**
   * Set this property to the desired logging level. In order of priority, available levels are:
   *
   * - 'fatal'
   * - 'error'
   * - 'warn'
   * - 'info'
   * - 'debug'
   * - 'trace'
   *
   * The logging level is a __minimum__ level. For instance if `logger.level` is `'info'` then all `'fatal'`, `'error'`, `'warn'`,
   * and `'info'` logs will be enabled.
   *
   * You can pass `'silent'` to disable logging.
   */
  level: LogLevelOrString;

  /**
   * Log at `'fatal'` level the given msg. If the first argument is an object, all its properties will be included in the JSON line.
   * If more args follows `msg`, these will be used to format `msg` using `util.format`.
   *
   * @typeParam T: the interface of the object being serialized. Default is object.
   * @param obj: object to be serialized
   * @param msg: the log message to write
   * @param ...args: format string values when `msg` is a format string
   */
  fatal: FastifyLogFn;
  /**
   * Log at `'error'` level the given msg. If the first argument is an object, all its properties will be included in the JSON line.
   * If more args follows `msg`, these will be used to format `msg` using `util.format`.
   *
   * @typeParam T: the interface of the object being serialized. Default is object.
   * @param obj: object to be serialized
   * @param msg: the log message to write
   * @param ...args: format string values when `msg` is a format string
   */
  error: FastifyLogFn;
  /**
   * Log at `'warn'` level the given msg. If the first argument is an object, all its properties will be included in the JSON line.
   * If more args follows `msg`, these will be used to format `msg` using `util.format`.
   *
   * @typeParam T: the interface of the object being serialized. Default is object.
   * @param obj: object to be serialized
   * @param msg: the log message to write
   * @param ...args: format string values when `msg` is a format string
   */
  warn: FastifyLogFn;
  /**
   * Log at `'info'` level the given msg. If the first argument is an object, all its properties will be included in the JSON line.
   * If more args follows `msg`, these will be used to format `msg` using `util.format`.
   *
   * @typeParam T: the interface of the object being serialized. Default is object.
   * @param obj: object to be serialized
   * @param msg: the log message to write
   * @param ...args: format string values when `msg` is a format string
   */
  info: FastifyLogFn;
  /**
   * Log at `'debug'` level the given msg. If the first argument is an object, all its properties will be included in the JSON line.
   * If more args follows `msg`, these will be used to format `msg` using `util.format`.
   *
   * @typeParam T: the interface of the object being serialized. Default is object.
   * @param obj: object to be serialized
   * @param msg: the log message to write
   * @param ...args: format string values when `msg` is a format string
   */
  debug: FastifyLogFn;
  /**
   * Log at `'trace'` level the given msg. If the first argument is an object, all its properties will be included in the JSON line.
   * If more args follows `msg`, these will be used to format `msg` using `util.format`.
   *
   * @typeParam T: the interface of the object being serialized. Default is object.
   * @param obj: object to be serialized
   * @param msg: the log message to write
   * @param ...args: format string values when `msg` is a format string
   */
  trace: FastifyLogFn;
  /**
   * Noop function.
   */
  silent: FastifyLogFn;
  child<O = ChildLoggerOptions>(bindings: Bindings, options?: O): FastifyBaseLogger
}

// TODO delete FastifyBaseLogger in the next major release. It seems that it is enough to have only FastifyBaseLogger.
/**
 * @deprecated Use FastifyBaseLogger instead
 */
export type FastifyLoggerInstance = FastifyBaseLogger

export interface FastifyLoggerStreamDestination {
  write(msg: string): void;
}

// TODO: once node 18 is EOL, this type can be replaced with plain FastifyReply.
/**
 * Specialized reply type used for the `res` log serializer, since only `statusCode` is passed in certain cases.
 */
export type ResSerializerReply<
  RawServer extends RawServerBase,
  RawReply extends FastifyReply<RawServer>
> = Partial<RawReply> & Pick<RawReply, 'statusCode'>;

/**
 * Fastify Custom Logger options.
 */
export interface FastifyLoggerOptions<
  RawServer extends RawServerBase = RawServerDefault,
  RawRequest extends FastifyRequest<RouteGenericInterface, RawServer, RawRequestDefaultExpression<RawServer>, FastifySchema, FastifyTypeProvider> = FastifyRequest<RouteGenericInterface, RawServer, RawRequestDefaultExpression<RawServer>, FastifySchema, FastifyTypeProviderDefault>,
  RawReply extends FastifyReply<RawServer, RawRequestDefaultExpression<RawServer>, RawReplyDefaultExpression<RawServer>, RouteGenericInterface, ContextConfigDefault, FastifySchema, FastifyTypeProvider> = FastifyReply<RawServer, RawRequestDefaultExpression<RawServer>, RawReplyDefaultExpression<RawServer>, RouteGenericInterface, ContextConfigDefault, FastifySchema, FastifyTypeProviderDefault>,
> {
  serializers?: {
    req?: (req: RawRequest) => {
      method?: string;
      url?: string;
      version?: string;
      hostname?: string;
      remoteAddress?: string;
      remotePort?: number;
      [key: string]: unknown;
    };
    err?: (err: FastifyError) => {
      type: string;
      message: string;
      stack: string;
      [key: string]: unknown;
    };
    res?: (res: ResSerializerReply<RawServer, RawReply>) => {
      statusCode?: string | number;
      [key: string]: unknown;
    };
  };
  level?: string;
  file?: string;
  genReqId?: (req: RawRequest) => string;
  stream?: FastifyLoggerStreamDestination;
}

export interface FastifyChildLoggerFactory<
  RawServer extends RawServerBase = RawServerDefault,
  RawRequest extends RawRequestDefaultExpression<RawServer> = RawRequestDefaultExpression<RawServer>,
  RawReply extends RawReplyDefaultExpression<RawServer> = RawReplyDefaultExpression<RawServer>,
  Logger extends FastifyBaseLogger = FastifyBaseLogger,
  TypeProvider extends FastifyTypeProvider = FastifyTypeProviderDefault
> {
  /**
   * @param logger The parent logger
   * @param bindings The bindings object that will be passed to the child logger
   * @param childLoggerOpts The logger options that will be passed to the child logger
   * @param rawReq The raw request
   * @this The fastify instance
   * @returns The child logger instance
   */
  (
    this: FastifyInstance<RawServer, RawRequest, RawReply, Logger, TypeProvider>,
    logger: Logger,
    bindings: Bindings,
    childLoggerOpts: ChildLoggerOptions,
    rawReq: RawRequest
  ): Logger
}

[flakey5]

Imho we have to decouple the pino types from the fastify logger types, especially the second parameter of child is using ChildLoggerOptions from the corresponding pino dependency

Wdyt of this @mcollina ?

[mcollina]

I concur, that was a bad idea (unfortunately)