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

[thw0rted]

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]

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

[kibertoad]

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

[kibertoad]

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

[thw0rted]

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]

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]

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]

go open a PR, it's easy to review

[Ethan-Arrowood]

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]

@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]

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]

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]

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]

Released in 7.5.0

[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.