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

IncomingMessage and ServerMessage typings are too specific #294

Closed
erunion opened this issue Aug 30, 2023 · 2 comments · Fixed by #295
Closed

IncomingMessage and ServerMessage typings are too specific #294

erunion opened this issue Aug 30, 2023 · 2 comments · Fixed by #295

Comments

@erunion
Copy link
Contributor

erunion commented Aug 30, 2023

We're gradually migrating our Express and Fastify hybrid codebase over to Typescript and have begun running into issues with various data we have in our Request objects:

pino({
  customProps: (req) => {
    return {
      '@type': 'http-request',
      '@service': 'readme',
      '@environment': process.env.NODE_ENV,
      '@timestamp': new Date().toISOString(),
      project: req.project?.subdomain ?? undefined,
    };
  },
})

The above code works normally in JS but in TS req.project is undefined because it does not exist on the customProps req parameter of IncomingMessage. We're already overloading the Express Request object to allow for req.proejct exist, but adding that, and everything else we're adding to that is a non-starter for us:

declare global {
  namespace Express {
    interface Request {
      project: ProjectDocument;
    }
  }
}

I have tried to type req as req: Request but even though Express' Request extends IncomingMessage the IncomingMessage typing on that is too specific:

error TS2769: No overload matches this call.
  Overload 1 of 2, '(opts?: Options | undefined, stream?: DestinationStream | undefined): HttpLogger', gave the following error.
    Type '(req: Request) => { '@type': string; '@service': string; '@environment': "backup" | "development" | "next" | "pr" | "preview" | "production" | "stage" | "test"; '@timestamp': string; project: string; }' is not assignable to type '(req: IncomingMessage, res: ServerResponse<IncomingMessage>) => object'.
      Types of parameters 'req' and 'req' are incompatible.
        Type 'IncomingMessage' is missing the following properties from type 'Request<ParamsDictionary, any, any, ParsedQs, Record<string, any>>': get, header, accepts, acceptsCharsets, and 56 more.
  Overload 2 of 2, '(stream?: DestinationStream | undefined): HttpLogger', gave the following error.
    Argument of type '{ customProps: (req: Request) => { '@type': string; '@service': string; '@environment': "backup" | "development" | "next" | "pr" | "preview" | "production" | "stage" | "test"; '@timestamp': string; project: string; }; }' is not assignable to parameter of type 'DestinationStream'.
      Object literal may only specify known properties, and 'customProps' does not exist in type 'DestinationStream'.

 28   return pino({
             ~~~~~~
 29     customProps: (req: Request) => {
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
... 
 37     },
    ~~~~~~
 38   });

I have also attempted to monkeypatch the Options interface to allow us to type req and res as Request and Response but TS doesn't like that either:

declare module 'pino-http' {
  export interface Options {
    customProps?: ((req: Request, res: Response) => object) | undefined;
  }
}

Subsequent property declarations must have the same type. Property 'customProps' must be of type '((req: IncomingMessage, res: ServerResponse) => object) | undefined', but here has type '((req: Request, res: Response) => object) | undefined'. ts(2717)

It would be really nice if when calling pino() we could supply it with a generic of our Express and Fastify Request and Response interfaces and let that populate down into wherever IncomingMessage and ServerResponse are being used. Something like this:

pino<Request, Response>({
  customProps: (req) => { // <-- This is a `Request` object now.
    return {
      '@type': 'http-request',
      '@service': 'readme',
      '@environment': process.env.NODE_ENV,
      '@timestamp': new Date().toISOString(),
      project: req.project?.subdomain ?? undefined, // <-- `req.project` now works because `project` exists on `Request`.
    };
  },
});
@mcollina
Copy link
Member

+1, would you like to send a PR?

@erunion
Copy link
Contributor Author

erunion commented Aug 30, 2023

sure!

erunion added a commit to erunion/pino-http that referenced this issue Aug 30, 2023
The typings on the various instances of `IncomingMessage` and `ServerResponse`
are too strict when coupled with Express and Fastify `Request` instances that
have been extended for custom properties. This loosens them up by allowing the
main `pino` instance to be supplied a generic of the Request and Response
interfaces that you use in your application.

Fixes pinojs#294
mcollina pushed a commit that referenced this issue Sep 4, 2023
…#295)

The typings on the various instances of `IncomingMessage` and `ServerResponse`
are too strict when coupled with Express and Fastify `Request` instances that
have been extended for custom properties. This loosens them up by allowing the
main `pino` instance to be supplied a generic of the Request and Response
interfaces that you use in your application.

Fixes #294
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 a pull request may close this issue.

2 participants