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
Fix types for pino.multistream
#1152
Conversation
could you add tests for the types? They live in: https://github.com/pinojs/pino/tree/master/test/types |
Added tests. How about the first argument? Should we change the types to more closely align with the actual implementation, or just stick to the documented JS usage? |
Thanks
I'm lost. I think the types are ok as is, but maybe I'm missing some detail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Well, in terms of the implementation, pino.multistream(process.stdout) // Single stream
pino.multistream([process.stdout]) // Array of streams
pino.multistream({ stream: process.stdout, level: 'info' }) // Single StreamEntry
pino.multistream([{ stream: process.stdout, level: 'info' }]) // Array of StreamEntries
// Mixed array
pino.multistream([
process.stdout,
{ stream: process.stderr, level: 'error' },
]) However, only the "Array of StreamEntries" usage is documented. So the TypeScript types in this PR only allows array of StreamEntries. Question is whether the types should allow any or all of the other cases. |
I think we should document all signatures and allow them in TS. Could you get them in? |
I have added the other signatures to the types and tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
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. |
Fixes TypeScript types for
pino.multistream
.Also, the implementation of
pino.multistream
is actually pretty loose with the first argument. It can handle:stream
and optionallylevel
properties (i.e. aStreamEntry
)write
method (i.e. aDestinationStream
)However, only array of
StreamEntry
is documented, and the current types reflect that. I don't know if that is intentional, so I have not changed that for now. If not, then the correct type should be(DestinationStream | StreamEntry)[] | DestinationStream | StreamEntry