Add prettyPrint option to logger type definitions #2550

agramian · 2020-09-10T06:44:32Z

The FastifyLoggerOptions interface is missing a type definition for the prettyPrint option which pino supports when pino-pretty is installed. The latest general Logging docs already reference the prettyPrint option however without the type definition an error is shown when using TypeScript and passing the option to the logger when creating the Fastify instance.

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

types/logger.d.ts

kibertoad · 2020-09-10T15:32:52Z

Could you also add type test for this? See https://github.com/fastify/fastify/tree/master/test/types

agramian · 2020-09-10T17:22:19Z

Could you also add type test for this? See https://github.com/fastify/fastify/tree/master/test/types

Yes. Will do.

zekth

LGTM

L2jLiga · 2020-09-11T07:05:17Z

types/logger.d.ts

@@ -1,6 +1,7 @@
 import { FastifyError } from 'fastify-error'
 import { RawServerBase, RawServerDefault, RawRequestDefaultExpression, RawReplyDefaultExpression } from './utils'
 import { FastifyRequest, RequestGenericInterface } from './request'
+import { PrettyOptions } from 'pino'


I'm a bit worried about TypeScript users. Will it work when user does not have @types/pino package installed?

pino does not provides own types and they shipped via @types/pino package hence If I won't install @types/pino TypeScript will consider typings as invalid

So would you prefer to just copy the type definition over? That way would mean it would have to be updated here on changes from pino / @types/pino.

Alternatively a note could be added to the docs. In order to use pino-pretty with fastify you need to install the pino-pretty dependency regardless so in the docs a note can be added to also install @types/pino if you are using TypeScript.

I would include typings in pino itself.
BTW moving @types/pino to dependencies or adding notes to README also acceptable, but as a WebStorm IDE user I would prefer first (moving @types/pino) for better smart completion.

I don't like including types that are not under our control (from @types/pino) to the fastify dependency tree.
@mcollina what do you think? What about a PR to pino itself to add typings? @jsumners any thoughts?

As you've learned, I 1. have these TS issues muted and 2. I have opinions. Opinions you will not appreciate.

@jsumners I appreciate very much your opinions actually. Even more, if your opinions are different from mine. That's why I tagged you here and why I ask your point of view whenever I can.

However, if you don't want to be tagged on TS issues, I am not going to tag you in the future.

That would be fine. I cannot provide any useful input for TS related things. If you want the full story, read through #649

I've just finished reading it. That issue was a really long ride.

mcollina

It should not depend on pino types, as stated multiple times in a lot of previous PRS on this topic. Maybe we can add a comment in the file?

mcollina · 2020-09-11T15:37:25Z

types/logger.d.ts

@@ -1,6 +1,7 @@
 import { FastifyError } from 'fastify-error'
 import { RawServerBase, RawServerDefault, RawRequestDefaultExpression, RawReplyDefaultExpression } from './utils'
 import { FastifyRequest, RequestGenericInterface } from './request'
+import { PrettyOptions } from 'pino'


Pino has no official types. No one of the maintainers of pino has enough typescript skills to have types. The types on @types are unofficial and should not be used.

mcollina · 2020-09-14T14:21:27Z

This is easy to fix by lifting whatever parts of the pino typedefs is needed in fastify itself. it's easy and swift.

Ethan-Arrowood · 2020-09-14T14:26:34Z

I'm not interested in maintaining more types than Fastify's so I'd rather we keep them here. Copy-pasting pino types and shipping them ourselves from within Fastify is okay as the pino api is not too complex.

What I've tried to articulate in the past is that there exists a minimal logger api that most fastify users use. When a dev needs more than that minimal interface; we should recommend them to install the pino types and use them in their fastify project themselves. I can't remember if they need to use declaration merging or use a generic to achieve that. This is the best path that doesn't add any additional maintenance responsibility to the projects in question

fox1t · 2020-09-14T14:27:09Z

This is easy to fix by lifting whatever parts of the pino typedefs is needed in fastify itself. it's easy and swift.

If we are fine in "duplicating" this over here, we should maintain Pino's typings in Fastify's codebase.

fox1t · 2020-09-14T14:29:36Z

I'm not interested in maintaining more types than Fastify's so I'd rather we keep them here. Copy-pasting pino types and shipping them ourselves from within Fastify is okay as the pino api is not too complex.

What I've tried to articulate in the past is that there exists a minimal logger api that most fastify users use. When a dev needs more than that minimal interface; we should recommend them to install the pino types and use them in their fastify project themselves. I can't remember if they need to use declaration merging or use a generic to achieve that. This is the best path that doesn't add any additional maintenance responsibility to the projects in question

We the types we write diverge in any non-mergiable way from @types/pino, basically we creating conflicts to our users that might be difficult to spot.

If we are all fine with this, then I vote too to add them to Fastify. :)

Ethan-Arrowood · 2020-09-14T14:31:32Z

We the types we write diverge in any non-mergiable way from @types/pino

this is why I think we should only ship a minimal viable type api for the logger. if the user wants full pino types they should be instructed to npm i -D @types/pino and then pass the Pino type to their FastifyInstance

fox1t · 2020-09-14T14:37:46Z

We the types we write diverge in any non-mergiable way from @types/pino

this is why I think we should only ship a minimal viable type api for the logger. if the user wants full pino types they should be instructed to npm i -D @types/pino and then pass the Pino type to their FastifyInstance

Are you suggesting adding a new generics to Fastify typings? 🤔

If so, we might have conflicts anyway if both types are decalred using the same namespace - interface name.

Ethan-Arrowood · 2020-09-14T14:40:50Z

https://github.com/fastify/fastify/blob/master/types/instance.d.ts#L20 Generic already exists for users to pass their own logger instance (i.e. Pino). There is even a test case: https://github.com/fastify/fastify/blob/master/test/types/logger.test-d.ts#L46

One aspect I would like to see changed in v4 is that we move the logger generic to the front of the list so that users don't have to specify the server, request, and reply ones as well. (We could try to use a generic object like the requestgenericinterface but iirc that breaks some of the magical inference so it might not be an actual solution here).

fox1t · 2020-09-14T14:43:41Z

Now I am even more confused than before. I know you can already pass a generic to "inject" pino's typings, but this PR is trying to fix a different issue. Am I wrong?

I am referring to this: https://github.com/fastify/fastify/blob/master/test/types/logger.test-d.ts#L4

P.S. Using object for generic would brake inference for sure as of today :(

Ethan-Arrowood · 2020-09-14T14:47:43Z

The inject method for pino typings is the solution to this issue. By recommending the user to inject pino typings they should get access to the prettyPrint property

fox1t · 2020-09-14T14:57:52Z

OK let me recap this for clarity, just to see if I understood what you are saying.
You are suggesting is to NOT add a prettyPrint as a Fastify's factory option in the typings and to let the options being injected by Fastify user? Are we going to remove other typings from here https://github.com/fastify/fastify/blob/master/types/logger.d.ts#L43

If not, why some options are "integrated" and others must be provided by passing explicitly Pino typings?

Sorry for asking this again, but I have a really hard time understanding this. :)

Ethan-Arrowood · 2020-09-14T15:04:15Z

Sorry for asking this again, but I have really hard time getting this. :)

No worries thank you for working through this with me!

You are suggesting is to NOT add a prettyPrint as a Fastify's factory option in the typings and to let the options being injected by Fastify user?

Yes

Are we going to remove other typings from here

No

If not, why some options are "integrated" and others must be provided by passing explicitly Pino typings?

Because of the idea of minimal viable api. We should provide a valid, minimal version of the logger types that is easy for us to maintain and will enable a basic TypeScript fastify user to have logging support in their app. If the user needs more than the provided basic api they need to install and inject the Pino types themselves.

My explanation here is already implemented as far as I know. It may need some small changes as well as better documentation. If others disagree with the way it is now then that is okay and we can continue having this discussion

mcollina · 2020-09-15T13:09:45Z

My explanation here is already implemented as far as I know. It may need some small changes as well as better documentation. If others disagree with the way it is now then that is okay and we can continue having this discussion

I think this is the most important part of this discussion. I don't understand why this PR is needed at all. Can we ease this scenario?

fox1t · 2020-09-15T13:15:23Z

@mcollina this is the first time that an option is more complex than a single value: prettyPrint could be an object. To avoid duplicating its types in fastify, this PR tried to import pino's typings and to use it here. It is the same as we do, for example, in the fastify-helmet type definitions. The difference is that helmet is shipping its types.

One possible way of simplifying it is to duplicate in this repo also the prettyPrint object types.

mcollina · 2020-09-15T13:16:18Z

One possible way of simplifying it is to duplicate in this repo also the prettyPrint object types.

Let's do this.

fox1t · 2020-09-15T13:30:43Z

One possible way of simplifying it is to duplicate in this repo also the prettyPrint object types.

Let's do this.

@agramian You can copy the object interface from here: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/72c9bd83316bd31e93ab86d64ddf598d922f33cd/types/pino/index.d.ts#L514

agramian · 2020-09-15T17:26:54Z

One possible way of simplifying it is to duplicate in this repo also the prettyPrint object types.

Let's do this.

@agramian You can copy the object interface from here: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/72c9bd83316bd31e93ab86d64ddf598d922f33cd/types/pino/index.d.ts#L514

Ok I lifted the interface and placed it just above the FastifyLoggerOptions interface where it is used. Let me know if you prefer the new interface to be renamed or relocated as I'm not sure what your preferences are.

A tweet relevant to this whole discussion. 😂

Ethan-Arrowood · 2020-09-15T17:27:35Z

Please add a non-jsdoc comment above the interface that says where it was copied from. Additionally, I think now is the best time for us to add a comment at the top of the logger file that summarizes the current decision on why we don't include @types/pino in the types.

That tweet is now 7 years old and it is more true than ever. Thank you for sharing 😂❤️

…rface

… from @types/pino

agramian · 2020-09-15T19:28:51Z

I added the comment referencing the copy source as well as the summary of the decision as best I could understand it from your guys' discussion.

Ethan-Arrowood

I love this. Great work on the comment at the top of the file ❤️

Thank you all for collaborating and discussing this issue!

mcollina

lgtm

L2jLiga

LGTM!

mcollina · 2020-09-18T07:01:28Z

@fox1t @kibertoad @Eomm are there any outstanding concerns before we land this?

fox1t

LGTM

fox1t · 2020-09-18T07:18:42Z

@fox1t @kibertoad @Eomm are there any outstanding concerns before we land this?

This is perfectly fine. If the pino public interface will ever change we will update these types. 👌

kibertoad · 2020-09-19T18:54:52Z

refs #2395

github-actions · 2022-02-07T00:13:38Z

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.

Add type to logger for prettyPrint

391b839

zekth requested changes Sep 10, 2020

View reviewed changes

types/logger.d.ts Outdated Show resolved Hide resolved

agramian added 2 commits September 10, 2020 23:32

Update prettyPrint option type to accept boolean or PrettyOptions

a63f4bc

Add type tests for logger prettyPrint option

399b014

zekth approved these changes Sep 11, 2020

View reviewed changes

L2jLiga reviewed Sep 11, 2020

View reviewed changes

Eomm added the typescript TypeScript related label Sep 11, 2020

mcollina requested changes Sep 11, 2020

View reviewed changes

fox1t mentioned this pull request Sep 14, 2020

Proposal: bundle type definitions in Pino itself pinojs/pino#910

Closed

Copy type definition for PrettyOptions from pino into fastify logger

208e3b2

Add non-jsdoc comment referencing source of copied PrettyOptions inte…

f51499a

…rface

Add comment in logger type def explaining rationale for not importing…

45ceedb

… from @types/pino

Ethan-Arrowood approved these changes Sep 16, 2020

View reviewed changes

mcollina approved these changes Sep 16, 2020

View reviewed changes

L2jLiga approved these changes Sep 18, 2020

View reviewed changes

fox1t approved these changes Sep 18, 2020

View reviewed changes

Eomm approved these changes Sep 18, 2020

View reviewed changes

mcollina merged commit f320619 into fastify:master Sep 18, 2020

github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 2022

Add prettyPrint option to logger type definitions #2550

Add prettyPrint option to logger type definitions #2550

Conversation

agramian commented Sep 10, 2020

Checklist

kibertoad commented Sep 10, 2020

agramian commented Sep 10, 2020

zekth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina commented Sep 14, 2020

Ethan-Arrowood commented Sep 14, 2020

fox1t commented Sep 14, 2020

fox1t commented Sep 14, 2020

Ethan-Arrowood commented Sep 14, 2020

fox1t commented Sep 14, 2020

Ethan-Arrowood commented Sep 14, 2020

fox1t commented Sep 14, 2020 • edited

Ethan-Arrowood commented Sep 14, 2020

fox1t commented Sep 14, 2020 • edited

Ethan-Arrowood commented Sep 14, 2020 • edited

mcollina commented Sep 15, 2020

fox1t commented Sep 15, 2020

mcollina commented Sep 15, 2020

fox1t commented Sep 15, 2020 • edited

agramian commented Sep 15, 2020

Ethan-Arrowood commented Sep 15, 2020 • edited

agramian commented Sep 15, 2020

Ethan-Arrowood left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

L2jLiga left a comment

Choose a reason for hiding this comment

mcollina commented Sep 18, 2020

fox1t left a comment

Choose a reason for hiding this comment

fox1t commented Sep 18, 2020

kibertoad commented Sep 19, 2020

github-actions bot commented Feb 7, 2022

fox1t commented Sep 14, 2020 •

edited

fox1t commented Sep 14, 2020 •

edited

Ethan-Arrowood commented Sep 14, 2020 •

edited

fox1t commented Sep 15, 2020 •

edited

Ethan-Arrowood commented Sep 15, 2020 •

edited