Proposal: bundle type definitions in Pino itself

[fox1t]

References

I found some prior talkings about this: #344

Background

I am opening this issue since we have currently a problem in the Fastify codebase: fastify/fastify#2550

TLDR;

Fastify relies on Pino for logging and therefore also Fastify's typing has to reference Pino's one. The issue is that at the present day they aren't maintained by Pino's team and we are not going to include 3rd party typings in Fastify codebase.

Possible solution

If the Pino core team agree I can actively maintain typings inside this repo with the help of some other fastify core team members:

• Add prettyPrint option to logger type definitions fastify/fastify#2550 (comment)
• Add prettyPrint option to logger type definitions fastify/fastify#2550 (comment)

I am opening this issue because far as I can see, we don't have any other viable solution for the issue fastify/fastify#2550

[jsumners]

Please describe what this would look like. Let's pretend I add a new hook in the same vein as the logMethod hook:

pino/docs/api.md

Lines 172 to 196 in db651a5

	<a id="logmethod"></a>
	##### `logMethod`
	Allows for manipulating the parameters passed to logger methods. The signature
	for this hook is `logMethod (args, method) {}`, where `args` is an array
	of the arguments that were passed to the log method and `method` is the log
	method itself. This hook ***must*** invoke the `method` function by using
	apply, like so: `method.apply(this, newArgumentsArray)`.
	For example, Pino expects a binding object to be the first parameter with an
	optional string message as the second parameter. Using this hook the parameters
	can be flipped:
	```js
	const hooks = {
	logMethod (inputArgs, method) {
	if (inputArgs.length >= 2) {
	const arg1 = inputArgs.shift()
	const arg2 = inputArgs.shift()
	return method.apply(this, [arg2, arg1, ...inputArgs])
	}
	return method.apply(this, inputArgs)
	}
	}
	```

Are you going to come along and update the types before the PR is committed? After? If after, do we now have to delay releases until a TS person does their thing?

How do these TS pull requests get reviewed? I can't review them. Matteo probably can't effectively review them. Likely not David either.

Why are these types even needed? From https://web.archive.org/web/20200912101819/https://www.typescriptlang.org/:

That says to me you shouldn't ever need some TS specific file to use Pino, regardless of where you use Pino.

[mcollina]

I'm ok in adding native types if we have 2 people willing to come on board and review them, keeping them as a separate thing (similar to fastify).

[fox1t]

I think that the process might be very similar to Fastify's one: we can schedule a release and then work all together on delivering it.

Speaking on the screenshot you provided, it isn't applicable here because Fastify already ships the typings, so we are already effectively responsible for providing the correct ones. We can just put unknown as Pino types, but it is the worst possible developer experience and we should avoid it, if possible.

[jstewmon]

👋 Hi, I'm willing to review & maintain type definitions. DefinitelyTyped is works well for what it is, but the separation between package maintainers and type definition maintainers results in perpetual drift and inaccuracies, so (as a pino & TS user / contributor) I think this is a worthwhile investment.

[kibertoad]

I am willing to participate in review and maintenance as well.

[davidmarkclements]

I'm fine with this as long as

• out of sync/lack of TS definitions do not block PR's
• if an issue regarding TS definitions is opened and not addressed and closed out in 30 days, we remove the definitions - and release that removal in a minor (since TS itself is fine with breaking changes in minor I don't think this is outlandish)

[jsumners]

• if an issue regarding TS definitions is opened and not addressed and closed out in 30 days, we remove the definitions - and release that removal in a minor

/highfive

[Ethan-Arrowood]

if an issue regarding TS definitions is opened and not addressed and closed out in 30 days, we remove the definitions - and release that removal in a minor

This is a very interesting policy idea. I wonder how it could help other projects that are hesitant to adopt native type support (node-tap comes to mind). I'll be keeping an eye on how it goes for Pino 😄

---

I also want to add one note: why not the few here that are willing to maintain Pino types do so in DefinitelyTyped? The testing and release infrastructure already exists and it seems like there is a lengthy list of existing contributors. Might be better to collaborate with them on DT than start again here.

[kibertoad]

@Ethan-Arrowood Personally I hate the whole flow of contributing to DefinitelyTyped, but then again, I'm not a fan of monorepos in general.

[Ethan-Arrowood]

@kibertoad understandable, but it has gotten better. They've put a lot of work into the bot so things are more automatic. If you have any direct feedback please let me know and I can forward it along

[kibertoad]

@Ethan-Arrowood Automation is not an issue, I would say that the whole concept of having to checkout a gajillion of folders to fix one interface, as well as having a crowded issue/pr space feels very messy to me.

It was quite intimidating to many developers on my team who are not to used to contributing to OSS, from which I can derive there must be a non-zero number of developers out there who might have contributed, but didn't, because from the outside it looks complex.

Also from developer experience perspective, not having another dependency to worry about is definitely convenient.

[Ethan-Arrowood]

Also from developer experience perspective, not having another dependency to worry about is definitely convenient.

totally agree

It was quite intimidating to many developers on my team who are not to used to contributing to OSS

this is good feedback and perspective

having to checkout a gajillion of folders to fix one interface

Try using partial clone/checkout next time (https://git-scm.com/docs/partial-clone)

[kibertoad]

@Ethan-Arrowood I use WebStorm for everything Git. Thanks for the reference, will check if it's supported from IDE. It definitely would make a DT experience more pleasant.

[jsumners]

There's no answer for this that satisfies everyone:

1. The @types project is a pox that makes life miserable for maintainers who couldn't care less about TS.
2. Adding types here will annoy the hell out of at least one maintainer.
3. The TS project either lies (see above) or the people writing TS are incapable of making that language work as advertised so they will complain to no end at JavaScript projects that don't cater to them.

It's a shitshow no one maintains JavaScript projects asked for.

[jstewmon]

@jsumners I can see how the popularity of TypeScript and DefinitelyTyped can be a bane to pure JS maintainers due to the growing expectations that users have for type definitions to be available for popular packages, placing an unfair expectation on package maintainers to provide type definitions...

Ultimately, developers want a tight feedback loop that helps them use packages correctly.

JSDoc has long been a popular way for package authors to provide this feedback to editors (and to automatically generate API documentation). TypeScript has even recently added support for generating type definitions from JSDoc comments.

TypeScript gives a lot of us leverage over large applications, not just while editing, but by having checks run during CI, we save a lot of time by preventing hard to catch mistakes in interface usage from slipping through. That leverage doesn't hinge on a single package, but it does accumulate package by package.

Given that pino doesn't already have JSDoc comments, I wouldn't suggest going that route simply to avoid TypeScript type definitions, but if that's something you'd find more appealing by sharing the benefits with pure JS consumers, it might be worth considering.

[fox1t]

Since I am the one who opened this issue, I think I need to clarify some points.

I perfectly understand why seeing an issue titled "Proposal: bundle type definitions in Pino" isn't the best issue a maintainer could find on the project board: if he/she/they cared about typings, the types would be there already or at least there would be an issue opened by the maintainer, asking for help.

However, in this specific case, the issue that led to this proposal was raised in Fastify. I here because @mcollina suggested me to ask here if there was any interest in adding types directly to the project.

Fastify team agrees that we don't want Fastify to rely upon a package (@types/pino) we don't have control over. At the moment, to get around this problem, as @Ethan-Arrowood pointed out, we are shipping a "minimal viable type API for the logger". Since Pino is the logger used by Fastify and there is some overlap between the two teams, it seemed natural to me to ask if I and others could help to maintain Pino's types: it was basically a win-win situation to me.

Moreover, @types/pino is downloaded 191,826 times per week (https://www.npmjs.com/package/@types/pino) and I saw this as a great opportunity to provide a better developer experience to developers that prefer to use TypeScript.

@Ethan-Arrowood, of course, we should help with @types/pino too but it will not resolve the issue that led to this proposal. This all started because of the "we don't want Fastify to rely upon a package (@types/pino) we don't have control over" part and maintaining @types/pino wouldn't fix this problem, unless we agree it will. :/

@davidmarkclements I am absolutely fine with both of your points! Types haven't to be a point of friction for the maintainers that don't care about them. :)

As a final note, I want to address @jsumners concerns:

1. it makes life miserable also to maintainers/developers who care about TS since @types is just an imperfect fallback that more often than not brings a ton of problems;
2. I am sorry to annoy you. I swear it wasn't my intention. It all started when I saw the Fastify's issue and I realized this could be one of the possible solutions;
3. As I already said, your assumption isn't correct for this specific case. Documentation doesn't lie and the people aren't incapable of making the language work as advertised. Since Fastify ships its typings, that paragraph isn't applicable to our own case: if there weren't any shipped types by fastify, we wouldn't have anything to fix and I wouldn't make this proposal. At the moment we are shipping partial typings: this is worse than not having them at all.

If adding types here isn't viable then we should probably just close this issue and accept that the best possible developer experience isn't achievable.

Again, sorry for bothering you all. Since I see some hatred about TS, next time I will be more careful in opening this kind of issues. Peace and love! ✌️

[jsumners]

If you can show me how to correctly write JSDoc for factory functions and the composed objects they return, I'd be all for that. But I'll be quite shocked if you can show it's possible.

[jsumners]

I see a two to one vote in favor of inclusion. One vote in favor suggests a viable way to trial the inclusion.

FYI: I had no idea that conversation was happening in the Fastify issues because I mute every TS related issue that comes in as soon as I see it.

[fox1t]

I see a two to one vote in favor of inclusion. One vote in favor suggests a viable way to trial the inclusion.

FYI: I had no idea that conversation was happening in the Fastify issues because I mute every TS related issue that comes in as soon as I see it.

That's why I included that issue here for reference. :)

[Ethan-Arrowood]

Good points @fox1t I'm always +1 for types, but I've learned it can be a point of contention too.

As I originally stated I think @davidmarkclements idea is really good! I'm looking forward to seeing how things develop 😁

[jsumners]

I think at this point it's down to whomever wants to do it to submit the PR.

[thw0rted]

I found this issue because I just updated from an older Pino that didn't bundle types, to 7.x, which does.

The current typings define a single pino export, so my old code that used import pino, {destination, stdSerializers} from "pino" no longer typechecks. I checked node_modules/pino/pino.js, and it still seems to include a bunch of module.exports.xyz = ... lines at the end, so I think the types should include those as separate module exports.

I see some discussion about whether or not to use a namespace export in the linked PR (#913) but it doesn't seem to have been resolved before merging. The general guidance from the TS team is not to use namespaces if you don't have to, when designing a new API. That's a good idea, but more importantly the current types do not reflect the actual shape of the module you're shipping. Right now, import {destination} from "pino" will run just fine in a Node .mjs file, but fails the TS type-checker, which means the types are wrong.

It's not a big deal for me -- I wrap most of my pino usage with some helper functions, so I only had to change one file in my project -- but I wanted to make sure the team knew the impact this has on your TS-based users. I can file a new issue if you like.

[jsumners]

@thw0rted please open a new issue. This one should have been closed at the types were added.

cc: @kibertoad

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.