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

fix: flush thread-stream missing callback #1137

Merged
merged 3 commits into from
Sep 21, 2021

Conversation

climba03003
Copy link
Contributor

@climba03003 climba03003 commented Sep 21, 2021

Fixes #1136

Currently, .flush will be either sonic-beam stream or thread-stream stream.
sonic-beam stream do not require callback for .flush. But, thread-stream does.

I just put a noop callback inside the function to prevent throwing for thread-stream.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

CI seems to have collapsed because some module moved to esm.

@climba03003
Copy link
Contributor Author

climba03003 commented Sep 21, 2021

CI seems to have collapsed because some module moved to esm.

CI fails because of file.js importing a non-exist file.

pino/file.js

Line 4 in 21ee5e2

const pino = require('../pino')

I have see the coverage report, it never include the file file.js.

@climba03003
Copy link
Contributor Author

climba03003 commented Sep 21, 2021

Missed the esm problem for pino-pretty.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit 0e8ecd6 into pinojs:master Sep 21, 2021
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pino 7 - error on flush
2 participants