search

strapi-community / strapi-plugin-rest-cache

Speed-up HTTP requests with LRU cache.
https://strapi-community.github.io/strapi-plugin-rest-cache/
MIT License
129 stars 29 forks source link

maxAge = 3600000 (41 days) is not a sensible default value #98

Closed pinkasey closed 1 month ago

pinkasey commented 1 month ago

https://github.com/strapi-community/strapi-plugin-rest-cache/blob/main/packages/strapi-plugin-rest-cache/server/types/CacheRouteConfig.js

image

In fact, it has caused me a serious bug in production. A sensible value would be 3600, which is 1 hour, which is what I think the author has intended

derrickmehaffy commented 1 month ago

Max age is in Ms which 3,600,000ms is one hour.

pinkasey commented 1 month ago

right.

I'm sending it as response header, so that causes me troubles. I assumed maxAge would be in seconds. I see the code now.

pinkasey commented 1 month ago

I suggest you change the definition of maxAge on version 5.x.x (it is a breaking change) - to seconds instead of milliseconds.

Also - document the units. it's not specified in the docs image

(also the 2 other places where you can config it) image

Boegie19 commented 1 month ago

TTLs are almost always in MS

derrickmehaffy commented 4 weeks ago

Yeah with the exception of DNS TTL values, in almost any other context a TTL would be in Ms not S

pinkasey commented 4 weeks ago

The field is called maxAge, which in the context of http (max-age directive in Cache-Control) is measured in seconds.

Honestly, when I start using any field that measures time, I try to understand what units it's expecting, I usually don't make assumptions. This time I failed.

It's your package, and if you don't want the documentation to be clear that's your choice.

derrickmehaffy commented 4 weeks ago

Documentation absolutely should be clear on this, while I don't own the package I am still one of the maintainers and completely refactoring the docs is on the todo