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
fix: make childLoggerFactory use FastifyBaseLogger #5244
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test for this? We use tsd.
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. This means that pino's |
a46f690
to
b7ed34c
Compare
What would you recommend we do? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
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 |
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 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 Second thing I tried was removing the requirement for the logger to extend 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 const instance: FastifyInstance = fastify({ logger: pino() }) I needed to change the generics of 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. |
Go ahead and make that change. cc @fastify/typescript |
Can you check that 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. |
It is, I'll commit the test I made for that
Just tried and it didn't seem to help
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 ( I think it might be possible that 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) |
Does it work even if you don't assign any logger? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}
Wdyt of this @mcollina ? |
I concur, that was a bad idea (unfortunately) |
Fixes #4960
Checklist
npm run test
andnpm run benchmark
and the Code of conduct