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 #699

Closed

Conversation

0xslipk
Copy link
Contributor

@0xslipk 0xslipk commented Dec 3, 2021

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

NOTE: I installed pino-http from master branch, because I am waiting for the next release.

Add support for default and name export with TypeScript

@0xslipk
Copy link
Contributor Author

0xslipk commented Dec 3, 2021

Hi @iamolegga please take a look of this PR. let me know if you have any question.

@iamolegga
Copy link
Owner

Hello @jarcodallo at first
image

Second, I'm just wondering do we really need to change the code of the current library instead of fixing types in parent libs – since types are moved from separate packages I thought that all those bugs are existing because of broken exports, which could be fixed and work pretty same as before just imported underhood from the same package as the lib, instead of separate types package.

Third, why do we need pino in peer deps, as it goes as a direct dependency of pino-http?

Forth, of course, now it's unsafe to merge this with github link instead of fixed npm version.

Thanks

@0xslipk
Copy link
Contributor Author

0xslipk commented Dec 3, 2021

Hello @jarcodallo at first image

Second, I'm just wondering do we really need to change the code of the current library instead of fixing types in parent libs – since types are moved from separate packages I thought that all those bugs are existing because of broken exports, which could be fixed and work pretty same as before just imported underhood from the same package as the lib, instead of separate types package.

Third, why do we need pino in peer deps, as it goes as a direct dependency of pino-http?

Forth, of course, now it's unsafe to merge this with github link instead of fixed npm version.

Thanks

First: resolved.
Second: Pino v7.5 removed the export for BaseLogger, because if you want to extends the logger the correct way to do it is using the Logger type that includes the BaseLogger and EventEmitter class implementation.
Third: we need it, because @nestjs/platform-fastify uses pino@6.x
image

Forth: Yeah we need to wait for the release version. 🚀

Thanks for the fast reply.

@iamolegga
Copy link
Owner

  1. I'm not sure. I still can see this message. I believe this is due to updates in both commits in package-lock file. I suggest just maybe start from the latest master?
  2. Am I correct that this is a breaking change in a minor release?) Can you please point me to the commit with this changes?
  3. Am I correct that this is devDeps tree? So for end-user of this lib there will not be any need for this, or not? I still didn't get it.
  4. Ok then, that wasn't clear for me at the start 😀

@0xslipk
Copy link
Contributor Author

0xslipk commented Dec 3, 2021

  1. I'm not sure. I still can see this message. I believe this is due to updates in both commits in package-lock file. I suggest just maybe start from the latest master?
  2. Am I correct that this is a breaking change in a minor release?) Can you please point me to the commit with this changes?
  3. Am I correct that this is devDeps tree? So for end-user of this lib there will not be any need for this, or not? I still didn't get it.
  4. Ok then, that wasn't clear for me at the start 😀

1.- I did haha. I don't see any warning.
image
2.- I don't think is a breaking change, BUT! using a major release of pino in your dependencies can be a reason to bump the major version of nestjs-pino. 6 days ago pino team merged a PR#1195 that moved the BaseLogger outside of the namespace. They just merged a PR#1246 that exported it the BaseLogger again, so we need to wait for the release and I will update this PR.
3.- You are correct, but the library won't compile, because it will try to use the fastify pino that needs @type/pino and this devDep is not longer necessary.
4.- Sorry for the misunderstood :D

@iamolegga
Copy link
Owner

  1. I think that's because you've merged upstream master to your branch after making changes. May I ask you to either squash all commits and force-push it or create a new branch from the latest master and make changes on top of it? Moreover, somehow it's not running tests on ci at the moment (that's behaviour from the old version).

image

2.

using a major release of pino in your dependencies can be a reason to bump the major version of nestjs-pino

not sure about that, because there are not any breaking changes, neither in API of the current library nor in API of pino logger: you should remove @types/pino from your project – nothing about changed types usage.

  1. If you say that it's required for compilation, why do we need it as peer dep instead of dev dep (which is already set)?

@0xslipk 0xslipk mentioned this pull request Dec 13, 2021
@0xslipk
Copy link
Contributor Author

0xslipk commented Dec 13, 2021

@iamolegga I opened a new PR #710

@0xslipk 0xslipk closed this Dec 13, 2021
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

2 participants