🐛 Add ability to cache comments count endpoint #15734
Conversation
Codecov ReportBase: 53.40% // Head: 53.42% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #15734 +/- ##
==========================================
+ Coverage 53.40% 53.42% +0.01%
==========================================
Files 1502 1502
Lines 97935 97950 +15
Branches 10976 10985 +9
==========================================
+ Hits 52307 52332 +25
+ Misses 44369 44360 -9
+ Partials 1259 1258 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
lenabaidakova
force-pushed
the
2094-inability-to-cache-comments-count-endpoint-due-to-post-setup
branch
4 times, most recently
from
5ed0e10 to
43a64ec
Compare
lenabaidakova
force-pushed
the
2094-inability-to-cache-comments-count-endpoint-due-to-post-setup
branch
from
43a64ec to
5470287
Compare
lenabaidakova
changed the title
| @@ -140,6 +140,9 @@ | |||
| }, | |||
| "contentAPI": { | |||
| "maxAge": 0 | |||
| }, | |||
| "comments": { | |||
There was a problem hiding this comment.
I do like the contentAPI convention used above. This is an API we are caching, so maybe commentsAPI would be a better fit. Also, with the discussion on the issue itself if we end up only caching the comments endpoint long-term, than commentsCountAPI would be even better name.
There was a problem hiding this comment.
Renamed it to commentsCountAPI
|
|
||
| const bodyParser = require('body-parser'); | ||
| const membersService = require('../../../server/services/members'); | ||
|
|
||
| module.exports = function apiRoutes() { | ||
| const router = express.Router('comment api'); | ||
|
|
||
| router.use(shared.middleware.cacheControl('public', {maxAge: config.get('caching:comments:maxAge')})); |
There was a problem hiding this comment.
The issue we are solving here is for one specific endpoint and not the whole API. I think we would be most safe to apply this caching rule to the counts endpoint first to unblock the issue and do more research around correct approach for the whole Comments API
There was a problem hiding this comment.
Added cache just of counts API
lenabaidakova
force-pushed
the
2094-inability-to-cache-comments-count-endpoint-due-to-post-setup
branch
2 times, most recently
from
5cb07c1 to
721d94e
Compare
|
@naz , thank you for explanation 🙌 Fixed next issues:
|
| @@ -12,10 +12,18 @@ const isString = require('lodash/isString'); | |||
| * @param {'public'|'private'} profile Use "private" if you do not want caching | |||
| * @param {object} [options] | |||
| * @param {number} [options.maxAge] The max-age in seconds to use when profile is "public" | |||
| * @param {number} [options.staleWhileRevalidate] The stale-while-revalidate in seconds to use when profile is "public" | |||
lenabaidakova
force-pushed
the
2094-inability-to-cache-comments-count-endpoint-due-to-post-setup
branch
from
96cb5f2 to
9ce43e2
Compare
lenabaidakova
force-pushed
the
2094-inability-to-cache-comments-count-endpoint-due-to-post-setup
branch
from
9ce43e2 to
fbec822
Compare
Fixed :) |
closes TryGhost/Team#2094
- Comment counts request was changed from `post` to `get` to enable request caching.
closes TryGhost/Team#2094
This value can be used for non-crucial data with a `public` option. For example: `public, max-age=1, stale-while-revalidate=9`.
The idea behind this option is that the browser would cache the value for the number of seconds in `max-age` and would use it for the number of seconds in `stale-while-revalidate` until it gets a "validated response" from the server. The behaviour should be almost unnoticeable for the end user but would make a big difference in the amount of requests to server.
lenabaidakova
force-pushed
the
2094-inability-to-cache-comments-count-endpoint-due-to-post-setup
branch
from
fbec822 to
21cb7ef
Compare
lenabaidakova
deleted the
2094-inability-to-cache-comments-count-endpoint-due-to-post-setup
branch
closes TryGhost/Team#2094
PR to comments repo TryGhost/comments-ui#12