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

New bundled TypeScript typings do not expose various top-level exports #1193

Closed
thw0rted opened this issue Oct 27, 2021 · 13 comments · Fixed by #1195
Closed

New bundled TypeScript typings do not expose various top-level exports #1193

thw0rted opened this issue Oct 27, 2021 · 13 comments · Fixed by #1195

Comments

@thw0rted
Copy link
Contributor

@thw0rted thw0rted commented Oct 27, 2021

As I commented in #910, the bundled TypeScript type definitions do not allow import {destination, stdSerializers} from "pino", which is perfectly legal code.

Looking at pino.js (as shipped in v7.0.5), I see top level exports defined for the following variables:

destination
transport
multistream
final
levels
stdSerializers
stdTimeFunctions
symbols
version

Since these can be imported from the module, the typings should include definitions for them, to accurately describe the shape of the JS module.

While you're at it, consider removing the namespace "P" that wraps all your type and interface declarations, and exporting them directly. Namespace bundling is no longer considered a best practice in TS, and prevents the use of the (relatively recently introduced) import type syntax.

This has real-world consequences. If I want to write a library that takes a DestinationStream, writing import pino from "pino"; export function withStream(str: pino.DestinationStream) { ... } results in including the entire pino library at runtime, even though I'm not actually constructing a logger or using any of pino's runtime logic, while the exact same code with import type {DestinationStream} from "pino" instead, only requires the library at compile-time, and doesn't bundle the pino implementation.

@kibertoad
Copy link
Contributor

@kibertoad kibertoad commented Oct 27, 2021

Would you be open to send a PR that would address this?

@kibertoad
Copy link
Contributor

@kibertoad kibertoad commented Oct 27, 2021

Not sure if removing namespace is feasible without it being a breaking change, though.

@kibertoad
Copy link
Contributor

@kibertoad kibertoad commented Oct 27, 2021

@pinojs/typescript I assume our next window for any breaking changes is pino 8?

@thw0rted
Copy link
Contributor Author

@thw0rted thw0rted commented Oct 27, 2021

It might be feasible to have all the type/interface exports as top-level, and keep the namespace bundle for legacy support until the next breaking release. I'll take a quick look now.

@thw0rted
Copy link
Contributor Author

@thw0rted thw0rted commented Oct 27, 2021

My "quick look" was not so quick. I have a version that works exactly as I want it to, but only with esModuleInterop set to false. It's close of business here in Europe, I'll try again tomorrow.

@thw0rted
Copy link
Contributor Author

@thw0rted thw0rted commented Oct 28, 2021

OK, I think I have it working in https://github.com/thw0rted/pino/tree/issue-1193

Does someone want to look over my approach first, or should I just open a PR?

@mcollina
Copy link
Member

@mcollina mcollina commented Oct 28, 2021

go open a PR, it's easy to review

@Ethan-Arrowood
Copy link

@Ethan-Arrowood Ethan-Arrowood commented Oct 28, 2021

If types are invalid it should be considered a bug and fixed with an appropriate patch or minor depending how the project is using semver. If it has to do with exports then its a bit harder, but generally it should do its best to match how the project would be imported via JS.

@kibertoad
Copy link
Contributor

@kibertoad kibertoad commented Oct 28, 2021

@Ethan-Arrowood There are two parts to this issue. I would see adding missing exports as a bug and it can be addressed in a patch release. Adding namespace-less exports on top of existing ones are closer to semver minor in my book. Removing namespace altogether would definitely be a semver major.

@thw0rted
Copy link
Contributor Author

@thw0rted thw0rted commented Oct 29, 2021

In the linked PR, I think you could ship what I wrote as patch or minor, then remove the final export type { pino as P } as a breaking change (major). Using types from a namespace is discouraged now, as I understand it, so eventually we'd want consumers to change import {P} from "pino"; function f(log: P.Logger){..} to import type {Logger} from "pino"; etc.

@KoltesDigital
Copy link

@KoltesDigital KoltesDigital commented Nov 9, 2021

import type {Logger} from "pino"; worked in 6.x but not in 7.x anymore. Bringing it back should be a patch ... and of course I'd like to have it released as soon as possible so that I don't have to temporarily switch to P 🤗 . I guess many users of this library are in this case. I also think you can leave P with a deprecated mention, I guess few users switched.

FTR I found this thread because I'm upgrading a project's dependencies and tsc complains about non-existing Logger type, whereas it worked before.

@Retro64
Copy link

@Retro64 Retro64 commented Nov 12, 2021

I stumbled over the same lost exported top-level Logger type when upgrading to the new version. I agree it should be a patch, as adding another top-level export does not mean the export within the namespace P needs to be dropped. It - for sure - becomes redundant, but won't be breaking (as it is for now for people, blindly upgrading the dependency to 7.x).

@kibertoad
Copy link
Contributor

@kibertoad kibertoad commented Nov 29, 2021

Released in 7.5.0

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 pull requests

Successfully merging a pull request may close this issue.

6 participants