Conversation
| .expect('Cache-control', 'public, max-age=1800'); // assert the Cache-control header is overwritten by the /v1/recommendations route | ||
|
|
||
| expect(res.status).toEqual(200); | ||
| expect(res.headers['cache-control']).toEqual('public, max-age=1800'); |
There was a problem hiding this comment.
I can either use the line 76 way, which is using the supertest library way of asserting on headers e.t.c or I can do it line 79 way which mirrors the existing pattern?
There was a problem hiding this comment.
Either one is fine. I don't think it's materially different.
| } | ||
| return next(); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
very very tiny refactor to get rid of the eslint / prettier errors. Works as expected but I wasn't sure the original intent behind not having a return and having nested arrow functions 🤔
Can revert
There was a problem hiding this comment.
This is equivalent.
It's just another syntax that can avoid nesting if your lint rules allow it.
Typically when I use higher order functions I would format it like this:
const CacheControlHandler =
(cachePolicy: string, config: typeof ConfigFile) =>
(req: Request, res: Response, next: NextFunction): void => {
...
The second function is interpreted as single expression and implicitly returns. It's nice and concise, but the eslint / prettier rules don't really like this at all though. Docs
| '/v1/recommendations', | ||
| // request must include a consumer | ||
| ConsumerKeyHandler, | ||
| CacheControlHandler('public, max-age=1800', config), |
There was a problem hiding this comment.
I don't have much knowledge around Web repo but I'm assuming this is where web repo sets the cache time limit for recommendations:
There was a problem hiding this comment.
That looks correct to me.
hyperparabolic
left a comment
hyperparabolic
left a comment
There was a problem hiding this comment.
I'd drop one of the cache-control checks in the test. I think this is good to go otherwise, pre-approved.
2 participants
Goal
Add CDN caching for fx proxy recommendations
I'd love feedback/perspectives on:
See review comments
References
JIRA ticket: