RFC: Response cache #985
RFC: Response cache #985
Conversation
🦋 Changeset detectedLatest commit: e1ae821 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
muningis
force-pushed
the
response-cache
branch
5 times, most recently
from
c42ca1c to
b8b1987
Compare
|
@yusukebe Hy, could you check this out please? :) Still very basic, no tests even yet, just to get feeling/idea if there would be interest to continue with this |
|
At first glance, I don't think this is needed. We already have the built-in Cache Middleware. If you want to cache just "Response", it's enough to use it. |
|
As per my understanding, that middleware only works on cloudflare an deno, and uses specific cache mechanism. This allows little bit more freedom, for example, by using Also, correct me if I'm wrong, but that Cache API helps cache'ing it on Cloudflare/DenoDeploy level, basically "edge/cdn" rather than on server, right? |
|
Forgot to mention - this allows to customize cache’ing keys. So for example, if you’re running A/B test, and store A/B tests list in cookie - you can use that cookie to allow to cache page rendered under specific A/B conditions. Quite niche example, but solution is quite lightweight with provided flexibility. |
|
Thank you for the explanation! Makes sense.
Yes. The Cache API is standardized, but you can only use it in particular runtimes like Cloudflare Workers and Deno. This middleware looks good. Let's go ahead. |
|
redis - or for example Cloudflare KV.. @muningis are you planning on finishing this PR? |
|
Had completely forgoten about this. I will get back to this next week. |
muningis
force-pushed
the
response-cache
branch
from
b8b1987 to
f02320b
Compare
…der filtering - Store full response (body, status, headers) instead of just body text - Filter sensitive headers (Set-Cookie, WWW-Authenticate, etc.) - Remove `respond` as it's inferred from `Content-Type` header - Add vitest tests
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #985 +/- ##
==========================================
+ Coverage 84.35% 84.46% +0.11%
==========================================
Files 119 120 +1
Lines 3879 3907 +28
Branches 1047 1054 +7
==========================================
+ Hits 3272 3300 +28
Misses 516 516
Partials 91 91
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
If this is ready to review, please ping me. |
|
Hello @yusukebe, yes this is ready now. |
|
Can you ping me if you fixed them? |
| if (cached) { | ||
| if (logging?.enabled) {logging.onHit?.(key, c)} | ||
|
|
||
| const snapshot = JSON.parse(cached) |
There was a problem hiding this comment.
I wonder if we need JSON parse here - maybe this logic could be in store as it can depend on storage service?
I.e. cloudflare workers cache api vs kv api - cache api returns Response, KV can return json or stream.
There was a problem hiding this comment.
@yusukebe what do you think? I'm inclined to agree, as this makes it more flexible, and perhaps even easier invalidation too.
|
@yusukebe - adjusted accordingly. |
|
I believe we can't merge this PR into the main because the branch has inconsistencies. Can you create another PR? Plus, can you set the project to match the other projects? For example, the |
|
@muningis If you get stuck, let me know 👋🏻 |
|
#1652 |
4 participants
The author should do the following, if applicable
yarn changesetat the top of this repo and push the changeset