fix: Support stale-while-revalidate

[richardgarnier]

This PR fixes #512
Note: this probably generate conflicts with #510

Also, I'm not completely convinced about the hydrate implementation. Currently, when we have a stale value we do the following:

• call the hydrate function with the stale data
• refetch the data and return result with cached === false

To me, this should be the opposite, thus:

• return the stale data with cached === true (when stale-while-revalidate allows us to do that)
• refetch the data, and call the hydrate function with the fresh data (if set)

This would allow for RFC compliance while also allowing users to get the fresh data as soon as it is available should they need it (when used on a server it does not necessarily make sense to use the double return technique).
However, should we want to make the change above, I think the current PR can still be merged and the change above be made in another PR.

[codecov]

Codecov Report

Merging #514 (0ff1cc3) into main (473f11c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #514   +/-   ##
=======================================
  Coverage   99.81%   99.81%           
=======================================
  Files          17       17           
  Lines         528      544   +16     
  Branches      166      174    +8     
=======================================
+ Hits          527      543   +16     
  Misses          1        1           

Impacted Files	Coverage Δ
src/header/interpreter.ts	100.00% <100.00%> (ø)
src/interceptors/response.ts	100.00% <100.00%> (ø)
src/storage/build.ts	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[arthurfiorette]

Hey @richardgarnier and @Embraser01, thanks for your work! I'll be pointing out some things that I think should change, but I'm open to discussion :)

[arthurfiorette]

To me, this should be the opposite, thus:

• return the stale data with cached === true (when stale-while-revalidate allows us to do that)
• re fetch the data, and call the hydrate function with the fresh data (if set)

With that, we may introduce a problem due to misleading usage. Not anyone who uses axios-cache-interceptor should understand all behaviors. By doing this, their axios calls will return stale data when it shouldn't. He already expects that axios() will do a network travel for him, cache is only a benefit when possible.

As not using the hydrate option don't throw any warnings (and it shouldn't), the end-user will only have fresh data (>> That we have in cache <<) when he fetches again. I know that stale-while-revalidate means exactly this, but for implementations that can re-execute the code when the fresh data arrives, and we cannot resolve a js promise two times.

This will lead to an behavior in a UI similar to the following:

• Uses axios.get() without hydrate and after the cache expired its needed to do 2 page reloads. 1 that returns immediately, does the network call asynchronously and saves it into the cache. And other that returns immediately with the newly cache.

• Uses axios.get() with hydrate and do the exactly above but with only 1 refresh that calls the provided hydrate and re-render the UI automatically. The problem with this approach is that re introduces the callback hell for every axios call and this is not good at all for our DX.

That's why I created the hydrate option, it tells you like: The cache is not valid at all, we will fetch a new one and resolve this promise with it, IF YOU REALLY want to avoid any UI flickering or letting the screen empty for some ms, you can use the hydrate option.

I'm ok to changing the hydrate name to another one if it is confusing, but inverting hydrate and response behavior does more harm than good.

[arthurfiorette]

Sorry if I was a bit strict :) open to discussions if you want :)

[richardgarnier]

Thanks for the review.

To me, this should be the opposite, thus:

• return the stale data with cached === true (when stale-while-revalidate allows us to do that)
• re fetch the data, and call the hydrate function with the fresh data (if set)

With that, we may introduce a problem due to misleading usage. Not anyone who uses axios-cache-interceptor should understand all behaviors. By doing this, their axios calls will return stale data when it shouldn't. He already expects that axios() will do a network travel for him, cache is only a benefit when possible.

As not using the hydrate option don't throw any warnings (and it shouldn't), the end-user will only have fresh data (>> That we have in cache <<) when he fetches again. I know that stale-while-revalidate means exactly this, but for implementations that can re-execute the code when the fresh data arrives, and we cannot resolve a js promise two times.

This will lead to an behavior in a UI similar to the following:

• Uses axios.get() without hydrate and after the cache expired its needed to do 2 page reloads. 1 that returns immediately, does the network call asynchronously and saves it into the cache. And other that returns immediately with the newly cache.
• Uses axios.get() with hydrate and do the exactly above but with only 1 refresh that calls the provided hydrate and re-render the UI automatically. The problem with this approach is that re introduces the callback hell for every axios call and this is not good at all for our DX.

That's why I created the hydrate option, it tells you like: The cache is not valid at all, we will fetch a new one and resolve this promise with it, IF YOU REALLY want to avoid any UI flickering or letting the screen empty for some ms, you can use the hydrate option.

I'm ok to changing the hydrate name to another one if it is confusing, but inverting hydrate and response behavior does more harm than good.

I disagree with this interpretation, because stale-while-revalidate explicitly allows reusing the value as if it was fresh. If treating the data as fresh while it is stale is a mistake, then the mistake is on the server side, and not on the implementation of the cache strategy.
Instead, having access to the revalidated response should be the bonus, and not the other way around, since the request fired is only intended to refresh the cache and NOT to notify the client (he already got his response from cache, since the server allowed us to do exactly this).
For anyone that does not look how the library works in detail, it is reasonable expectation that the library will behave in accordance with the specification. Please note, that this library is not necessarily used on the front-end 😄.

For reference: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#stale-while-revalidate

Sorry if I was a bit strict :) open to discussions if you want :)

No worries.
As a side-note, I don't personally mind either way since we can achieve what we need by using hydrate.

[arthurfiorette]

Suuuuper thanks for your time in this PR! Sorry that it took too long (I moved to a state 930 miles away) for me to review.

[arthurfiorette]

About the hydrate discussion, took a time to search about it and still din't find a way to solve it with good DX. I'm searching second opinions and implementations out there, like http-cache-semantics.