fastify/fastify-caching

Only set cache for 200, 301 and 302 response

adityapatadia opened this issue · 7 comments

adityapatadia commented

Currently cache-control header is sent for even 404 response. Can we avoid it?

mcollina commented

Thanks for reporting this!

It definitely should, would you like to send a PR?

You should add a check for the status codes in https://github.com/fastify/fastify-caching/blob/master/plugin.js#L43-L52.

mcollina commented

I think that should move to a onSend hook instead.

adityapatadia commented

Well I thought about it and current one seems fine to me. I set cache control to private by default and when I need the response to be cached, I set the header with reply.header. If we move it there, there must be a check that it should not set cache-control header if something in the app has already set it.

mcollina commented

Yes exactly.

prevostc commented

I did run into an issue with this plugin caching 429 responses from https://github.com/fastify/fastify-rate-limit
Still trying to bypass this behavior.

@mcollina I would like to contribute a PR for this. What option do you most like

  • add a config to enable/disable caching for specific http codes
  • add a new config like "shouldCache: (req, reply) => boolean"
  • only set cache headers only if they have not been set
  • something else?
mcollina commented

only set cache headers only if they have not been set

I think this is the best option.

  • 👍1