-
Notifications
You must be signed in to change notification settings - Fork 13
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
Pino 7 upgrade #13
Pino 7 upgrade #13
Conversation
Co-authored-by: James Sumners <james@sumners.email>
Not sure it matters since this should be a new major, but tests are failing on 12. |
We are still supporting v12 in pino@7. |
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
Then tests should pass. |
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.
I think this needs to embed pino-socket. This module is supposed to be used with it and transports cannot be piped together.
They are independent modules. We don't know what people do with the socket module. It's really, really, generic and can be used for many things even outside of Pino logs. It should remain a standalone module. Instead, we should be able to pipe transports into one another. |
I meant to add that functionality here, not viceversa as this module is supposed to work with pino-socket. |
I will try to integrate it |
t.teardown(() => { | ||
serverSocket.close() | ||
serverSocket.unref() | ||
transport.end() |
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.
I had to add this line to make it works on node12.
I thought it was due the duplexyfy
stream, that accepts these two parameters:
autoDestroy: true,
destroy: true
But it was not.
Nether the underlining socket.unref()
was the issue.
I would like to understand why the transport doesn't close.
I still don not figured it out.
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.
What was the problem? why the test did not pass? It might be a bug in my code.
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.
tap
triggers a timeout error - locally within an infinity timeout, the process does not end without any errors
@Eomm pino@7.0.0-rc.8 is out, you can use update this and use |
Done |
There is something else I can do on this PR? |
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
@jsumners anything left for you? |
Ref pinojs/pino#1056