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

Bump pino version from 6.0.0 to 7.0.0 #144

Merged
merged 6 commits into from Nov 3, 2021

Conversation

arty-name
Copy link
Contributor

fixes #143

@mcollina
Copy link
Collaborator

please remove node 10

@arty-name
Copy link
Contributor Author

Sure! Since Node 16 became LTS recently, I have also added it to the matrix, is that fine?

@mcollina
Copy link
Collaborator

@mcollina
Copy link
Collaborator

I think we can drop old API versions.

@arty-name
Copy link
Contributor Author

I think we can drop old API versions.

What do you mean? Hapi 18 and 19?

@mcollina
Copy link
Collaborator

Yes, exactly

@arty-name
Copy link
Contributor Author

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: The following leaks were detected:AggregateError, atob, btoa, AbortController, AbortSignal, EventTarget, Event, MessageChannel, MessagePort, MessageEvent, performance. Do you have an idea about addressing that?

@mcollina
Copy link
Collaborator

I think you need to update lab.

Can you also update the README?

@arty-name
Copy link
Contributor Author

Indeed, the latest lab version does not have this issue!

@arty-name
Copy link
Contributor Author

I am uncertain about this comment though, since it mentions a different package pino-http.

@mcollina
Copy link
Collaborator

The same block of code must be added here for everything to work.

@coveralls
Copy link

coveralls commented Oct 30, 2021

Coverage Status

Coverage decreased (-0.1%) to 98.976% when pulling 096e842 on arty-name:fix-143-upgrade-pino into 7e9e699 on pinojs:master.

@arty-name
Copy link
Contributor Author

Do you mean something like this?

Copy link
Collaborator

@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
Collaborator

@lauraseidler can you take a look as well?

This is good to go for me.

@lauraseidler
Copy link
Collaborator

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.

@mcollina
Copy link
Collaborator

mcollina commented Nov 1, 2021

They are testable but likely something we should not really bring here. I'd be ok in landing as is.

@mcollina mcollina merged commit 61e5d3e into hapijs:master Nov 3, 2021
@arty-name arty-name deleted the fix-143-upgrade-pino branch November 3, 2021 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to pino@^7.0.0
4 participants