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 Pino HTTP types #710

Merged
merged 1 commit into from Dec 14, 2021

Conversation

0xslipk
Copy link
Contributor

@0xslipk 0xslipk commented Dec 13, 2021

This PR will fix the problem with pino and pino-http types.

#699
pinojs/pino-http#175

@0xslipk
Copy link
Contributor Author

0xslipk commented Dec 13, 2021

Hi @iamolegga please take a look of this PR. I did all the changes as we discussed it in #699

@iamolegga
Copy link
Owner

Hey @jarcodallo , thanks! It looks like it needs to be rebased again (thanks, dependabot) 😅
image
And I'm still wondering do we really need to include pino in deps here as it goes as dep of pino-http. I'm going to fork your branch and exclude it from the deps to see the result for the test project if it will work without pino as a direct dep of this lib

@0xslipk
Copy link
Contributor Author

0xslipk commented Dec 14, 2021

Hey @jarcodallo , thanks! It looks like it needs to be rebased again (thanks, dependabot) 😅 image And I'm still wondering do we really need to include pino in deps here as it goes as dep of pino-http. I'm going to fork your branch and exclude it from the deps to see the result for the test project if it will work without pino as a direct dep of this lib

OMG! remove the deps bot hahaha (flipping the table meme here).
I included it, because the @nestjs/platform-fastify install an old version of pino that need the types from @types/pino package. I will check if everything work w/o pino in devDeps. Thanks

@0xslipk
Copy link
Contributor Author

0xslipk commented Dec 14, 2021

@iamolegga I removed pino from the devDeps and get this error:
image

With pino installed as devDep:
image

@iamolegga
Copy link
Owner

Ok then, let's keep pino in deps, I believe we can drop it when platform-fastify will be updated.

Thank you very much for this improvement! Merging!

@iamolegga iamolegga merged commit b6043d5 into iamolegga:master Dec 14, 2021
@0xslipk
Copy link
Contributor Author

0xslipk commented Dec 14, 2021

Ok then, let's keep pino in deps, I believe we can drop it when platform-fastify will be updated.

Thank you very much for this improvement! Merging!

Happy to help 🚀

@iamolegga
Copy link
Owner

@jarcodallo now I'm thinking which version update should be for this update. major, minor, fix 🤔 API of this lib didn't change, but imports/types of underlying logger have changed slightly

@0xslipk
Copy link
Contributor Author

0xslipk commented Dec 14, 2021

@jarcodallo now I'm thinking which version update should be for this update. major, minor, fix 🤔 API of this lib didn't change, but imports/types of underlying logger have changed slightly

My personal opinion is that you shouldn't update the major version, just the minor, because currently v2.x is broken when it tries to install pino-http@v6 as peer dep.

@holm
Copy link

holm commented Dec 14, 2021

Thanks for fixing this. Looking forward to be abe to update pino and related libs.

I would also vote for a minor version change for the same reason as @jarcodallo

@iamolegga
Copy link
Owner

Yes, correct, will ship now, thanks, everyone!

@iamolegga
Copy link
Owner

please check out v2.4.0 🎉

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.

None yet

3 participants