Improve logger type declaration #1532
Conversation
| import * as http from 'http' | ||
| import * as http2 from 'http2' | ||
| import * as https from 'https' | ||
| import * as pino from 'pino' |
There was a problem hiding this comment.
We decided not to ship pino types. Pino does not have official types support and it relies on @types/pino.
There was a problem hiding this comment.
Yeah the import for it is the same. I downloaded @types/pino as devDep
There was a problem hiding this comment.
This was done on purpose and should be reverted. For Fastify v2 we decided not to ship these types.
|
@evanshortiss do you have any tips for how I can make use of the HttpRequest and HttpResponse generics? |
|
@Ethan-Arrowood I think it looks like your doing it the right way, though it might need to include export default interface FastifyLoggerOptions<
HttpRequest extends (http.IncomingMessage | http2.Http2ServerRequest) = http.IncomingMessage,
HttpResponse extends (http.ServerResponse | http2.Http2ServerResponse) = http.ServerResponse
> |
| genReqId?: string; | ||
| } | ||
|
|
||
| type WriteFn = (o: object) => void; |
There was a problem hiding this comment.
Would it make more sense if this was:
type WriteFn = (...args: any[]) => void;Since it's a pino logger you can pass multiple args? Or am I mistaken to believe this? For example, it should be valid to do:
fastify.log.info('here is your json %j', someJson)There was a problem hiding this comment.
The current types have two methods defined. I'm working on merging the two right now.
declare type WriteFn = (msg:string, ...args: any[]) => void;
declare type WriteFn = (obj: {}, msg?: string, ...args: any[]) => void;TypeScript says I can't do it this way so I'm working on it 😅
|
Thank you to everyone who has looked over this PR. I'm going to mark this as invalid for right now because I'm currently working on implementing these types in a different way (that I think is going to be better for a couple of reasons). More to come soon! |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@Ethan-Arrowood should this be closed? |
|
Yes go ahead and close this. My type refactor will incorporate this change.
…--
Ethan Arrowood
Computer Science & Applied Mathematics
Accelerate | HackWITus | ΦΣΠ
<https://github.com/Ethan-Arrowood> <https://twitter.com/ArrowoodTech> [image:
https://ethanarrowood.com] <https://ethanarrowood.com> [image:
https://www.facebook.com/ethan.arrowood]
<https://www.facebook.com/ethan.arrowood>
|
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Checklist
npm run testandnpm run benchmarkNot complete yet. I want to add a standalone typescript test file that tests the logger types I've defined. I think using the atomic approach to types declarations and testing will be the best way of improving type support across the project.
In this PR I have declared the types for the
fastifyinstanceloggeroption parameter. It was previously defined asany. I am not sure if this is considered breaking or not because:Furthermore on that second bullet, I'd like to update the logging documentation to be more explicit about what options users have for logging
a 'valid logger' could be better documented as the current line (in reference to custom serializers)