Issue #1193 - Typescript typings top level exports #1195

thw0rted · 2021-10-28T12:42:08Z

kibertoad · 2021-10-28T21:21:01Z

pino.d.ts


-/// <reference types="node"/>
+import type { EventEmitter } from "events";
+import type { PrettyOptions } from "pino-pretty";


Considering that pino-pretty is not a runtime dependency of pino, I don't think we should depend on its types. This would break builds for some of our users.

I'm having a hard time finding official guidance on how to handle situations like this.

Copy/pasting the types in from pino-pretty is asking for trouble, as they're going to change and you'll wind up with out of date information that breaks builds -- "type PrettyOptions [from pino] is not assignable to type PrettyOptions [from pino-pretty]" etc. I know that the way I've implemented it should be fine if consumers use the skipLibCheck flag, but it may cause problems if they don't.

I've seen several proposed solutions for this, like some kind of "peer dev dependencies" or "optional types", but AFAICT nothing has actually shipped.

I think we mostly leaned in the direction of inlining types for dependencies we don't already rely on in the past, so I would suggest doing the same here. However suboptimal it is, other solutions seem worse.

Based on this guidance, it sounds like the type import should gracefully degrade to any if pino-pretty isn't installed, which I think is the desired behavior -- if they aren't using the dependency, they shouldn't provide an argument for prettyOptions. Is that what you meant by "seem worse" or is there some other (mis-)behavior I'm not aware of?

I did test it and the behavior I described is accurate. I ran npm pack on my fork, then npm install typescript /path/to/pino-x.y.z.tgz in a new project. pino-pretty is not installed, and the language service shows the type of prettyOptions as any. If you run tsc without skipLibCheck, it flags the import type line above, but also 6 other imports (for node / NodeJS, http, events, and worker_threads). I get that pino is mostly a Node-focused library but it does have a browser option, and requiring that users include @types/node to use it can cause conflicts (e.g., W3C and Node have different global ReadableStream interfaces). IME, skipLibCheck is the least-worst solution.

I've cracked it! Adding a // @ts-ignore immediately before the import type line causes it to gracefully degrade to any when pino-pretty is not installed, even skipLibCheck=false. Installing pino-pretty causes incorrect options (like colorize: "true") to be flagged as an error, then you can uninstall pino-pretty and the code compiles cleanly. (That's correct, right? Without pino-pretty installed, prettyOptions should just be ignored?)

ETA: if providing pinoOptions when pino-pretty isn't installed should be an error, we can replace L358 with

prettyPrint?: any extends PrettyOptions ? false : (boolean | PrettyOptions);

This will flag calls that pass prettyPrint: { ... } or prettyPrint: true as errors, when pino-pretty isn't installed.

pino.d.ts

kibertoad · 2021-10-28T21:24:27Z

Impressive! Thank you for your contribution.

kibertoad · 2021-10-28T21:26:41Z

test/types/pino.test-d.ts

@@ -180,7 +180,8 @@ const pretty = pino({
        messageKey: "msg",
        timestampKey: "timestamp",
        translateTime: "UTC:h:MM:ss TT Z",
-        search: "foo == `bar`",
+        // Not actually a valid prettyPrint option(!)
+        // search: "foo == `bar`",


why keep it in a commented out form?

Oh I meant to label that as TODO -- I didn't research this, I assumed that it used to take a search param and doesn't anymore? Anyway, great example of the problems caused by copy/pasting types from the other library 😃

Do you still plan to revise this part?

I don't see anything about it over at pino-pretty, I'll just take it out.

This is still here?..

kibertoad · 2021-11-02T21:30:25Z

Can you add more tests for namespace-less imports?

thw0rted · 2021-11-03T10:32:01Z

I just noticed that I'd added some more tests last week but forgot to push them. I'm still not testing most of the type exports -- not exactly sure how that would work -- but it should include at least some of the functions and constants.

thw0rted · 2021-11-09T11:09:38Z

@kibertoad would you mind taking a look at this? @KoltesDigital just mentioned that it's holding up their upgrade, and I think the push I did last week probably addresses your previous feedback, though you might still want more top-level import tests. (I wasn't sure exactly what needs to be added.)

kibertoad · 2021-11-23T22:29:36Z

pino.d.ts

- */
-declare function P(optionsOrStream?: P.LoggerOptions | P.DestinationStream): P.Logger;
+type SerializerFn = (value: any) => any;
+type WriteFn = (o: object) => void;


are you sure you want object and not Record<string, any> or any here?

I didn't write that, just moved it around from the old typings. There are a lot of places where I don't think the types are optimal, but I wasn't going to try to fix them all.

kibertoad · 2021-11-23T22:32:07Z

@thw0rted I'm deeply sorry for the delay. Will do my best to review and merge quickly after remaining comments are addressed.

Speaking of top level export tests. Do we currently import all of the following in out types test?

destination
transport
multistream
final
levels
stdSerializers
stdTimeFunctions
symbols
version

? If not, we should, to ensure those exports are not broken in the future.

kibertoad · 2021-11-27T14:27:44Z

Thanks a lot!
@mcollina Can we ship it?

github-actions · 2022-11-28T00:04:30Z

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.

thw0rted added 3 commits October 27, 2021 17:15

Add top level exports to TypeScript defs

5d42da5

Restructure to work with esModuleInterop: true

6f37ab3

Fix whitespace, add a few top-level-export tests

ac5b943

mcollina requested a review from kibertoad October 28, 2021 13:01

kibertoad reviewed Oct 28, 2021

View reviewed changes

pino.d.ts Show resolved Hide resolved

kibertoad reviewed Oct 28, 2021

View reviewed changes

kibertoad reviewed Nov 23, 2021

View reviewed changes

PR feedback

f4ab73b

kibertoad approved these changes Nov 27, 2021

View reviewed changes

kibertoad merged commit 89f565a into pinojs:master Nov 27, 2021

climba03003 mentioned this pull request Nov 30, 2021

Remove app.use() fastify/fastify#3502

Closed

2 tasks

0xslipk mentioned this pull request Dec 3, 2021

Fix pino-http types iamolegga/nestjs-pino#699

Closed

github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2022

thw0rted deleted the issue-1193 branch November 28, 2022 00:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #1193 - Typescript typings top level exports #1195

Issue #1193 - Typescript typings top level exports #1195

thw0rted commented Oct 28, 2021

kibertoad Oct 28, 2021

thw0rted Oct 29, 2021

kibertoad Nov 23, 2021

thw0rted Nov 24, 2021

thw0rted Nov 24, 2021

thw0rted Nov 24, 2021 •

edited

Loading

kibertoad commented Oct 28, 2021

kibertoad Oct 28, 2021

thw0rted Oct 29, 2021

kibertoad Nov 2, 2021

thw0rted Nov 3, 2021

kibertoad Nov 23, 2021

kibertoad commented Nov 2, 2021

thw0rted commented Nov 3, 2021

thw0rted commented Nov 9, 2021

kibertoad Nov 23, 2021 •

edited

Loading

thw0rted Nov 24, 2021

kibertoad commented Nov 23, 2021

kibertoad commented Nov 27, 2021

github-actions bot commented Nov 28, 2022

Issue #1193 - Typescript typings top level exports #1195

Issue #1193 - Typescript typings top level exports #1195

Conversation

thw0rted commented Oct 28, 2021

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

thw0rted Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

kibertoad commented Oct 28, 2021

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

kibertoad commented Nov 2, 2021

thw0rted commented Nov 3, 2021

thw0rted commented Nov 9, 2021

kibertoad Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibertoad commented Nov 23, 2021

kibertoad commented Nov 27, 2021

github-actions bot commented Nov 28, 2022

thw0rted Nov 24, 2021 •

edited

Loading

kibertoad Nov 23, 2021 •

edited

Loading