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

Issue #1193 - Typescript typings top level exports #1195

Merged
merged 4 commits into from Nov 27, 2021

Conversation

@thw0rted
Copy link
Contributor

@thw0rted thw0rted commented Oct 28, 2021

Fixes #1193.

@mcollina mcollina requested a review from kibertoad Oct 28, 2021

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

@kibertoad kibertoad Oct 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@thw0rted thw0rted Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@kibertoad kibertoad Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@thw0rted thw0rted Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@thw0rted thw0rted Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@thw0rted thw0rted Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
@kibertoad
Copy link
Contributor

@kibertoad kibertoad commented Oct 28, 2021

Impressive! Thank you for your contribution.

@@ -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`",
Copy link
Contributor

@kibertoad kibertoad Oct 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why keep it in a commented out form?

Copy link
Contributor Author

@thw0rted thw0rted Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😃

Copy link
Contributor

@kibertoad kibertoad Nov 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you still plan to revise this part?

Copy link
Contributor Author

@thw0rted thw0rted Nov 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@kibertoad kibertoad Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still here?..

@kibertoad
Copy link
Contributor

@kibertoad kibertoad commented Nov 2, 2021

Can you add more tests for namespace-less imports?

@thw0rted
Copy link
Contributor Author

@thw0rted thw0rted commented Nov 3, 2021

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
Copy link
Contributor Author

@thw0rted thw0rted commented Nov 9, 2021

@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.)

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

@kibertoad kibertoad Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@thw0rted thw0rted Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@kibertoad kibertoad commented Nov 23, 2021

@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 kibertoad merged commit 89f565a into pinojs:master Nov 27, 2021
15 checks passed
@kibertoad
Copy link
Contributor

@kibertoad kibertoad commented Nov 27, 2021

Thanks a lot!
@mcollina Can we ship it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants