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

TypeScript issue when passing Pino instance to fastify constructor #4960

Open
2 tasks done
pavelkornev opened this issue Aug 8, 2023 · 14 comments · May be fixed by #5244
Open
2 tasks done

TypeScript issue when passing Pino instance to fastify constructor #4960

pavelkornev opened this issue Aug 8, 2023 · 14 comments · May be fixed by #5244
Labels
bug Confirmed bug typescript TypeScript related

Comments

@pavelkornev
Copy link

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

Plugin version

No response

Node.js version

18.14.2

Operating system

macOS

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

13.5

Description

By some historical reasons we pass a Pino instance to fastify factory function (I'm aware that Pino is built-in into fastify):

const logger = pino({
  // ...some config
});

const server = fastify({
  logger,
  // ...other opts
});

But starting v4.20.0 this code is no longer working with the following TypeScript compile error:

Type 'FastifyInstance<RawServerDefault, IncomingMessage, ServerResponse<IncomingMessage>, Logger<LoggerOptions | DestinationStream>, FastifyTypeProviderDefault>' is not assignable to type 'FastifyInstance<RawServerDefault, IncomingMessage, ServerResponse<IncomingMessage>, FastifyBaseLogger, FastifyTypeProviderDefault>'.
  The types of 'withTypeProvider().childLoggerFactory' are incompatible between these types.
    Type 'FastifyChildLoggerFactory<RawServerDefault, IncomingMessage, ServerResponse<IncomingMessage>, Logger<LoggerOptions | DestinationStream>, any>' is not assignable to type 'FastifyChildLoggerFactory<RawServerDefault, IncomingMessage, ServerResponse<IncomingMessage>, FastifyBaseLogger, any>'.
      The 'this' types of each signature are incompatible.
        Type 'FastifyInstance<RawServerDefault, IncomingMessage, ServerResponse<IncomingMessage>, FastifyBaseLogger, any>' is not assignable to type 'FastifyInstance<RawServerDefault, IncomingMessage, ServerResponse<IncomingMessage>, Logger<LoggerOptions | DestinationStream>, any>'.
          Types of property 'log' are incompatible.
            Type 'FastifyBaseLogger' is not assignable to type 'Logger<LoggerOptions | DestinationStream>

It seems to be related to this recent change - #4760

Steps to Reproduce

  1. Create Pino instance
  2. Try to pass to fastify factory function

Expected Behavior

No response

@pavelkornev pavelkornev changed the title TypeScript issue when pass Pino instance to fastify constructor TypeScript issue when passing Pino instance to fastify constructor Aug 8, 2023
@mcollina
Copy link
Member

mcollina commented Aug 9, 2023

Thanks for reporting!

Can you provide steps to reproduce? We often need a reproducible example, e.g. some code that allows someone else to recreate your problem by just copying and pasting it. If it involves more than a couple of different file, create a new repository on GitHub and add a link to that.

@ChrisLahaye
Copy link

We have the same issue but its because we are assigning the fastify instance to a variable with type FastifyInstance. Since we don't specify the generics the logger type defaults to FastifyBaseLogger, which doesn't seem to be compatible with any pino instance. Seems like the new FastifyBaseLogger type is not correct.

@pavelkornev
Copy link
Author

We have the same issue but its because we are assigning the fastify instance to a variable with type FastifyInstance. Since we don't specify the generics the logger type defaults to FastifyBaseLogger, which doesn't seem to be compatible with any pino instance. Seems like the new FastifyBaseLogger type is not correct.

Yeah, we also assign the server to a variable of type FastifyInstance and it fails starting v4.20.0.

@ziimakc
Copy link

ziimakc commented Aug 10, 2023

@mcollina this is example:

import { fastify, type FastifyInstance } from "fastify";
import { pino } from "pino";

// all latest versions
const log2 = pino();
const app2: FastifyInstance = fastify({
	logger: log2,
});

Maybe related to #4928

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 10, 2023

Why should it be related #4928?

@mcollina
Copy link
Member

@ziimakc I've literally tested your example with the top-most version of Fastify, Pino and TypeScript and I could not reproduce. Please provide a full repro including the tsconfig.json and all relevant assets.

@ziimakc
Copy link

ziimakc commented Aug 11, 2023

@mcollina here it is: https://github.com/ZiiMakc/fastify-type-errr

run:

pnpm i
pnpm lint

@mcollina
Copy link
Member

This is confirmed only with "strict": true in the tsconfig.json.

There seem to be an issue with:

Type 'FastifyBaseLogger' is not assignable to type 'Logger<LoggerOptions | DestinationStream>'

Which I think it's due to:

child(bindings: Bindings, options?: ChildLoggerOptions): FastifyBaseLogger

not being compatible with

https://github.com/pinojs/pino/blob/91a3505cfaef323f565e3cfe93f5d5077320faff/pino.d.ts#L83

I don't know why this problem shows up only in strict mode and how to change this. At a first glance these seems very similar. However some of those generics are slightly different.

Anyhow, this fixes your problem:

import { fastify, type FastifyInstance, type FastifyBaseLogger } from "fastify";
import { pino } from "pino";

// all latest versions
const log = pino();
const app: FastifyInstance = fastify({
  logger: log as FastifyBaseLogger,
});

@mcollina mcollina added bug Confirmed bug typescript TypeScript related labels Aug 17, 2023
@rdzidziguri
Copy link

We faced the same problem, and the only solution is to use the as keyword which is not ideal, but it does the job for now.

@NotEvenANeko
Copy link

In the

const log = pino();
const app: FastifyInstance = fastify({
  logger: log,
});

case, we are assigning FastifyInstance<..., pino.Logger, ...> to FastifyInstance<..., FastifyBaseLogger, ...>, so the type of FastifyInstance<..., pino.Logger, ...>.childLoggerFactory (or some other functions) need to be compatible with FastifyInstance<..., FastifyBaseLogger, ...>.childLoggerFactory.

And the childLoggerFactory function takes a this parameter as type FastifyInstance, so it means we need to assign a (this: FastifyInstance<..., pino.Logger, ...>): ... to (this: FastifyInstance<..., FastifyBaseLogger, ...>): ....

And function parameters in TypeScript are contravariant when strict: true or strictFunctionTypes: true, which means even though pino.Logger is the subtype of FastifyBaseLogger (FastifyBaseLogger is pino.BaseLogger with a child function), the function (this: FastifyInstance<..., pino.Logger, ...>): ... is the basetype of (this: FastifyInstance<..., FastifyBaseLogger, ...>): ..., and we can't assign a basetype to a subtype.

https://www.typescriptlang.org/docs/handbook/type-compatibility.html#comparing-two-functions

This should also work, app should be FastifyInstance<..., pino.Logger, ...> now.

const log = pino();
const app = fastify({
  logger: log,
});

@mcollina
Copy link
Member

How would you fix it?

@mcollina
Copy link
Member

@flakey5 could you take a look?

@flakey5
Copy link

flakey5 commented Nov 23, 2023

From my testing I believe I can second what @NotEvenANeko said about this.

It's not so much FastifyBaseLogger being incompatible with Logger<LoggerOptions | DestinationStream>, this works without error

const log = pino();
const a: FastifyBaseLogger = log;
const b: boolean | FastifyLoggerOptions<RawServerDefault> & PinoLoggerOptions | Logger = log; // type of the `logger` property in `FastifyOptions`

but instead pino.Logger becoming the base type rather than FastifyBaseLogger. So for example a function abc(logger: Logger) needs either an instance of a pino.Logger or something that extends pino.Logger.

It does appear to be connected to #4760, removing childLoggerFactory from the FastifyInstance type fixes the problem

Few things we can do to fix it (starting with good ending with bad),

  1. Swapping this line
    childLoggerFactory: FastifyChildLoggerFactory<RawServer, RawRequest, RawReply, Logger, TypeProvider>;
    for childLoggerFactory: FastifyChildLoggerFactory<RawServer, RawRequest, RawReply, FastifyBaseLogger, TypeProvider>; - Should be good, but it does throw off the precedent of just passing in Logger
  2. Replace this line
    Logger extends FastifyBaseLogger = FastifyBaseLogger,
    with Logger = any - Not great, decreased type safety and might be breaking
  3. Remove Logger from FastifyInstance's generic fields and always use FastifyBaseLogger - Not great, definitely breaking

@mcollina
Copy link
Member

@flakey5 would you like to send a PR for option 1?

flakey5 added a commit to flakey5/fastify that referenced this issue Dec 28, 2023
@flakey5 flakey5 linked a pull request Dec 28, 2023 that will close this issue
4 tasks
flakey5 added a commit to flakey5/fastify that referenced this issue Dec 28, 2023
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.

8 participants