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
Bump pino version from 6.0.0 to 7.0.0 #144
Conversation
please remove node 10 |
Sure! Since Node 16 became LTS recently, I have also added it to the matrix, is that fine? |
You need to do https://github.com/pinojs/pino-http/blob/f43143e7cca538ed185ee67affec008a9df840c0/logger.js#L161 as well. CI is still failing |
I think we can drop old API versions. |
What do you mean? Hapi 18 and 19? |
Yes, exactly |
I removed the older hapi versions, probably the Readme needs to be updated at some point. The tests fail for me only on Node 16 with this error: |
I think you need to update lab. Can you also update the README? |
Indeed, the latest lab version does not have this issue! |
I am uncertain about this comment though, since it mentions a different package pino-http. |
The same block of code must be added here for everything to work. |
Do you mean something like this? |
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
@lauraseidler can you take a look as well? This is good to go for me. |
Sorry for the delay folks - I only have my mobile phone at the moment, but from a quick glance this LGTM. Should we have a test for those lines added @mcollina ? Not sure if it's testable or why exactly it's needed. |
They are testable but likely something we should not really bring here. I'd be ok in landing as is. |
fixes #143