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

Pino 7 TypeScript #175

Closed
rstrand opened this issue Nov 3, 2021 · 12 comments
Closed

Pino 7 TypeScript #175

rstrand opened this issue Nov 3, 2021 · 12 comments

Comments

@rstrand
Copy link

rstrand commented Nov 3, 2021

Now pino 7 is including TypeScript types, would it make sense to include TypeScript types in this package as well? The types in DefinitelyTyped for this package don't work with the pino 7 types.

@mcollina
Copy link
Member

mcollina commented Nov 3, 2021

Would you like to send a Pull Request to address this issue? Remember to add unit tests.

cc @kibertoad

@rstrand
Copy link
Author

rstrand commented Nov 10, 2021

I don't have any experience adding types to a project. Would it be acceptable to copy the types from definitely typed and fix the errors?

@gkampitakis
Copy link
Contributor

Hello 👋 are there any updates on this issue? Are types going to be included in pino-http? I wouldn't mind trying and start a pr with your help

@mcollina
Copy link
Member

A PR would be nice.

@0xslipk
Copy link
Contributor

0xslipk commented Nov 18, 2021

A PR would be nice.

Hi @mcollina, I opened a PR with the type definition. let me know what do you think. 🚀

@oddeirik
Copy link
Contributor

oddeirik commented Nov 26, 2021

Am I missing some required changes after the typings now being included in the package itself?

The IncomingMessage declaration for a 'req' property is missing in types now, which means TS will complain about doing `req.log.info()' in Express handlers/middleware.

This module declaration was included in the typings from @types/pino-http, but does not exist in the typings included in pino-http itself now:
(from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/5bdfa971b6a8422f8fe1d7ea2a87f1f1e911a8fe/types/pino-http/index.d.ts#L66)

declare module 'http' {
    interface IncomingMessage {
        id: PinoHttp.ReqId;
        log: Logger;
    }

    interface ServerResponse {
        err?: Error | undefined;
    }

    interface OutgoingMessage {
        [PinoHttp.startTime]: number;
    }
}

@kibertoad
Copy link

@oddeirik Would you like to send a PR to fix this?

@umarov
Copy link

umarov commented Nov 30, 2021

#184 I think this one "removed" some PinoHttp.Options from types. They aren't being merged properly. I am using NestJS Pino Logger and using the redact option.

LoggerModule.forRoot({
      pinoHttp: [
        {
          redact: [
            'req.headers.authorization',
            'req.headers.cookie',
            'req.headers["x-csrf-token"]'
          ]
        }
      ]
    }),

It was yelling at me once I upgraded to the latest version. Moving back to 6.3.0 fixed it. I haven't figured out how that PR could have broken the types. I need to take a look again and see if I can make a PR.

@0xslipk
Copy link
Contributor

0xslipk commented Nov 30, 2021

#184 I think this one "removed" some PinoHttp.Options from types. They aren't being merged properly. I am using NestJS Pino Logger and using the redact option.

LoggerModule.forRoot({
      pinoHttp: [
        {
          redact: [
            'req.headers.authorization',
            'req.headers.cookie',
            'req.headers["x-csrf-token"]'
          ]
        }
      ]
    }),

It was yelling at me once I upgraded to the latest version. Moving back to 6.3.0 fixed it. I haven't figured out how that PR could have broken the types. I need to take a look again and see if I can make a PR.

Yeah. I'm working on a fix for this in nestjs-pino, but first I need #188 merge into master

@0xslipk
Copy link
Contributor

0xslipk commented Dec 3, 2021

@umarov I opened a PR in nestjs-pino to fix the problem.

@mbrevda
Copy link

mbrevda commented Jan 24, 2023

Was this ever resolved? Currently grappling with:
Screen Shot 2023-01-24 at 6 47 58 PM

@mcollina
Copy link
Member

This was done long ago. Please open a fresh issuez

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

No branches or pull requests

8 participants