clay / amphora

Middleware for Express that composes components into renderable pages
https://claycms.gitbooks.io/amphora/
MIT License
31 stars 23 forks source link

adds 'url' to log context for rendering errors #687

Closed mattoberle closed 3 years ago

mattoberle commented 3 years ago

When Amphora throws an error in a location where req/res context is available we don't currently include that context.

This commit includes url in the log context to make it easy to reproduce errors.


According to the express docs res.locals is available by default and there is no risk of attempting to reference a property on an undefined object under normal use.

There may be more useful context available, but this seems like a good place to start.

For example, res.locals.user would log which user initiated an action (for internal requests using amphora-auth): https://github.com/clay/amphora-auth/blob/94201d60a6043c968b507a66e7ab289ce4f0139b/index.js#L144-L153

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 2463


Totals Coverage Status
Change from base Build 2458: 0.0%
Covered Lines: 4197
Relevant Lines: 4197

💛 - Coveralls
mattoberle commented 3 years ago

lgtm, but would also be a bit safer to use lodash _get(res, 'locals.url')

@reubenson yeah I was thinking about that too. It looks like Express sets res.locals so it should be there: https://expressjs.com/en/api.html#res.locals

The only situation I can think of where it wouldn't would be if someone added a middleware to wipe out that attribute or set it to a non-object (which seems like a mis-use of Express and maybe should result in a hard error?).

Edit: We already import lodash in these modules, might as well do the safer thing. It's probably worth keeping the side-effect code (ie. logging) as safe as possible.