-
Notifications
You must be signed in to change notification settings - Fork 27
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
RFC: Koa-centric implementation - Provide an api with ctx instead of req/res #31
base: master
Are you sure you want to change the base?
Conversation
const maxInt = 2147483647 | ||
let nextReqId = 0 | ||
return function genReqId (ctx) { | ||
// TODO: decide on order of keys, this matches current behavior of checking req.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmealo I don't think we need to add support for id on ctx.state.
The logging middleware should be the first or after an error middleware.
Adding on ctx.req would be the same as other pino adapters.
@jmealo I've tested your plugin in my current project. Looks great. Child loggers used by the other middlewares receive the custom props and logs emitted does contain the custom props. |
@stephanebachelier: I'm having an issue with log volume. I do not find it helpful that logging in the ctx on a request duplicates the entire line. Looking to possibly diverge completely and merge log entries differently. Can you provide a failing test for your use case? |
@jmealo are you still wanting to push forward with this? |
I think the best way to support
koa
is to embrace it fully. I'm using this in a project for now but am open to becoming the maintainer of this package. This is what I came up with for now.