Open devinrhode2 opened 2 years ago
Why are you are double-casing a wrong parameter?
cc @kibertoad
I'm not sure what you mean
Double casing a wrong parameter
I assume this is the parameter you are referring to:
const level = 'debug'
const stream = someWriteSteam
pino(
// This param?
{ level, steam }
)
I don't know what you mean by "double casing"
I believe I originally encountered this bug by updating from pino.. 7.0.2? All the way to latest 7.11
It seems some older version of pino supported loggerOptions.stream
Do you know which version supported it? I'm mot convinced we ever did.
Anyway, would you like to send a Pull Request to address this issue? Remember to add unit tests.
Of the two ideas I Suggested both are pretty hard breaking changes, likely calling for a major version bump. I don't think it'd be very hard to make the change, the main dilemma would be the version bump.
Given you think stream was never a supported property, I have few questions -what if it was added? Something like "writeStream"? "pino" would then only accept one options object parameter
Which, brings into question, why was it setup as a separate parameter in the first place? Surely there will be something we can find via git blame
This is implementable without having any breaking change.
I don't know exactly why TS isn't creating a type error in my original example. I'm guessing because LoggerOptions type is too loose.. so if it's possible to more directly address that issue, that's the change we should do.
But in future major version, I think we should consider making stream a property
This does not produce a type error:
Probably the signature for
pino
is too complicated. Simplifying it will make writing the TS defs for it simpler (and easier to write+maintain).I'd suggest something like:
OR, make it one argument and just put everything into
LoggerOptions
:A simpler TS type def will probably be more reliable long term and also produce a type error in my situation.