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

Documentation: res.log() is only mentioned in quietReqLogger? #289

Closed
jmealo opened this issue Jul 20, 2023 · 8 comments · Fixed by #290
Closed

Documentation: res.log() is only mentioned in quietReqLogger? #289

jmealo opened this issue Jul 20, 2023 · 8 comments · Fixed by #290

Comments

@jmealo
Copy link
Contributor

jmealo commented Jul 20, 2023

Hello,

I was reading the API docs, and noticed that res.log() is only mentioned in quietReqLogger:

quietReqLogger: when true, the child logger available on req.log will no longer contain the full bindings and will now only have the request id bound at reqId (note: the autoLogging messages and the logger available on res.log will remain the same except they will also have the additional reqId property). default: false

Should we document res.log() elsewhere -- otherwise, it may give the impression of being a typo.

Thanks for the great work!

@jmealo jmealo changed the title Documentation: quietReqLogger: res.log() -> req.log()? Documentation: res.log() is only mentioned in quietReqLogger? Jul 20, 2023
@mcollina
Copy link
Member

Can you clarify? res.log() should not be a thing.

@jmealo
Copy link
Contributor Author

jmealo commented Jul 20, 2023

@mcollina: those were my thoughts, I initially thought it was a typo, and then I saw this in the code:
https://github.com/pinojs/pino-http/blob/master/logger.js#L150C1-L150C1

Perhaps at some point, an HTTP framework/abstraction gives you access to the res but not the req? ... but I'm having a hard time understanding why that'd be the case. 🤔

@mcollina
Copy link
Member

res.log is not a function

@jmealo
Copy link
Contributor Author

jmealo commented Jul 20, 2023

Sure, it's a reference to req.log, the issue being presented is:

  • Is res.log adequately documented?

  • Does anyone care if the only answer to this question is to read the source?

  • Does the single occurrence of res.log make it appear to be a typo?

@mcollina
Copy link
Member

Is res.log adequately documented?

Likely no, can you add it?

Does anyone care if the only answer to this question is to read the source?

I don't understand this question

Does the single occurrence of res.log make it appear to be a typo?

No, res.log is a valid usecase. There might be cases where we don't have a reference of req but only of res.

@jmealo
Copy link
Contributor Author

jmealo commented Jul 21, 2023

@mcollina: I can try to open a PR, trying to lobby to switch out Bunyan with Pino at my day job. I'm guessing that the typescript definitions for OutgoingMessage should add log so that you can reference res.log without encountering an error.

@jmealo
Copy link
Contributor Author

jmealo commented Jul 21, 2023

@mcollina: I have a PR ready, however, it appears there's an error on #master with the typescript types?

index.test-d.ts:140:3 - error TS2739: Type '{ id: string | undefined; method: string; url: string; headers: { header: string; }; remoteAddress: string; remotePort: number; raw: IncomingMessage; }' is missing the following properties from type 'SerializedRequest': params, query

140   req: {
      ~~~

  index.d.ts:68:5
    68     req: SerializedRequest;
           ~~~
    The expected type comes from property 'req' which is declared here on type 'StdSerializedResults'


Found 1 error in index.test-d.ts:140

@jmealo
Copy link
Contributor Author

jmealo commented Jul 21, 2023

It looks like the standard serializers type defs changed and params and query are mandatory. I added empty ones to fix the test, but, I'm not sure if they should be made optional or not.

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