Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support stale-while-revalidate and stale-if-error #27

Open
adamstep opened this issue Dec 17, 2019 · 8 comments
Open

Support stale-while-revalidate and stale-if-error #27

adamstep opened this issue Dec 17, 2019 · 8 comments

Comments

@adamstep
Copy link

adamstep commented Dec 17, 2019

Hi @kornelski, thanks for your work on this library, it looks fantastic.

For my use case, I would need the library to support these two Cache-Control directives. From MDN:

  • stale-while-revalidate=<seconds>: Indicates that the client is willing to accept a stale response while asynchronously checking in the background for a fresh one. The seconds value indicates for how long the client is willing to accept a stale response.
  • stale-if-error=<seconds>: Indicates that the client is willing to accept a stale response if the check for a fresh one fails.

I think it could be implemented in the following way:

  • satisfiesWithoutRevalidation() would check the stale-if-error directive and return true if the response is a 5XX and the stale response can be used.
  • A new method satisfiesWithRevalidation() would check the stale-while-revalidate directive and return true if the stale response can be used while revalidating in the background. The caller would be expected to make the request while also using the cached response.

If you're open to these enhancements, I'd be happy to work on them.

@kornelski
Copy link
Owner

kornelski commented Dec 17, 2019

It would be great to support these features. stale-if-error seems like a straighforward improvement of the current implementation.

stale-while-revalidate is the more interesting feature, but it doesn't fit the existing API. satisfiesWithRevalidation is not a good solution, since a cache would have to run both functions, so it's adding whole another evaluation just for taking one option into account.

Perhaps there could be a new method like whatDoIDoWithThis (needs a better name) that returns one of "use it", "use it, but revalidate", "don't use it, go to server", etc. (needs better names).

@adamstep
Copy link
Author

I'll work on stale-if-error first since it doesn't require a change to the API.

Maybe the new method could look like this:
const { use, revalidate } = policy.check(newRequest)

@kornelski
Copy link
Owner

Yeah, that looks good.

@adamstep
Copy link
Author

I started implementing this and realized that my proposal for stale-if-error doesn't work. satisfiesWithoutRevalidation looks at the new request before revalidation, whereas stale-if-error only applies if a validation attempt fails due to an error.

So I think this has to be done with a new method satisfiesIfError(newRequest), which would need to be called if satisfiedWithoutRevalidation(newRequest) returns false and the revalidation responses is a 50X.

@kornelski
Copy link
Owner

Ah, right. So together with stale-while-revalidate this requires rethinking how this interacts with caches.

@sithmel
Copy link
Contributor

sithmel commented Feb 9, 2020

I think stale-if-error should:

  • change the time to live of a policy
  • have no impact on satisfiesWithoutRevalidation
  • change the behaviour of revalidatedPolicy. On error we automatically get a modified=false, and the policy is not updated

What do you think? I am happy to help with the implementation

@kornelski
Copy link
Owner

If you're implementing it in parallel with an actual HTTP client I'm confident it'll make sense.

@sithmel
Copy link
Contributor

sithmel commented Feb 25, 2020

The implementation is ready here: #30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
@kornelski @sithmel @adamstep and others