Skip to content

Conversation

@Marcel-Wil
Copy link

Added Flexible caching with stale-while-revalidate (SWR) for the package. To see how to use it look at the added parts of Readme

Added tests to test new SWR feature

Copy link
Member

@freekmurze freekmurze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I like the changes, but I think (you can correct me if I'm wrong), that AI played a big part in creating this. Let's polish it some more, so it feels hand crafted.

README.md Outdated

The package supports flexible caching with stale-while-revalidate behavior using explicit middleware. With SWR, cached responses have two time periods:

- **Fresh period**: The cache is considered fresh and served immediately without revalidation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: nowhere in our docs, we use bold styling for highlighting parameters. I presume this was AI generated. Remove this bold styling and try to follow the stying of the rest of the readme.

README.md Outdated

### Flexible caching with stale-while-revalidate (SWR)

The package supports flexible caching with stale-while-revalidate behavior using explicit middleware. With SWR, cached responses have two time periods:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a high chance that people don't know what stale-while-revalidate is. We should explain better in human language what this exactly does. Try to give a nice example. Link to the Laravel docs so people can also check out Laravel's docs on flexible. Make it as if you explain this all to a newcomer 🙂

Carbon::setTestNow();
});

describe('fresh cache period', function () {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used this is AI generated. In our tests suites, we don't use describe, remove it.

I would be find with all the tests in one file, but if you feel it would be better split up, create a directory, flexibleCache with multiple test files.

use Spatie\ResponseCache\Events\ResponseCacheHit;

beforeEach(function () {
// Use array driver for time-based tests and tag support
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is unnecessary.

README.md Outdated

// Fresh for 3 minutes, then stale for 15 minutes
Route::get('/api/posts', 'PostController@index')
->middleware(CacheResponse::flexible(180, 900));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to know what does numbers are, let's use named arguments in this example to improve readability.

README.md Outdated
->middleware(CacheResponse::flexible(180, 900, false, 'posts', 'api'));
```

The `flexible()` method accepts:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of giving all examples first, and then explaining all parameters, it would be more readable to give a simple example, explain that fully. And then introduce another parameter, give an example for the new parameter and so on.

That will feel a lot more "human" that what we have now.

return 'dummy response';
})->middleware('cacheResponse:300');

// Flexible cache routes for SWR testing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this comment, it doesn't add any value. Instead use a Route group, in which you set the /flexible prefix.

$deferFlag = $defer ? '1' : '0';
$flexibleTime = "{$freshSeconds}:{$totalSeconds}:{$deferFlag}";

if (empty($tags)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not that readable.

We should build up a base string, and then if you use tags, concatenate the extra string needed to that base string.

* @param string ...$tags Optional cache tags
* @return string
*/
public static function flexible(int $freshSeconds, int $totalSeconds, bool $defer = false, ...$tags): string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not common to have a middleware with two public methods. Let's extract this to a dedicated middleware FlexibleCacheResponse. That will make it easier in usage, and the make the middleware simpler. Maybe it could extend of a BaseCacheMiddleware where common logic lives.

Copy link
Member

@freekmurze freekmurze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's moving in the right direction. 👍

Here are a few more nitpicks / ideas.


public function addCacheAgeHeader(Response $response): Response
{
if (config('responsecache.add_cache_age_header') and $time = $response->headers->get(config('responsecache.cache_time_header_name'))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't use and mostly. Replace it with &&

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually let's make that && go away, by adding a nested if, that will make it more readable.

$response = $this->getCachedResponse($request, $tags);
if ($response !== false) {
return $response;
if ($this->responseCache->enabled($request) && ! $this->responseCache->shouldBypass($request)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also refactor that && away by adding an extra nested if

}

$response->headers->set(config('responsecache.cache_age_header_name'), $ageInSeconds);
if (is_string($middleware) && str_starts_with($middleware, FlexibleCacheResponse::class.':')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nested ifs are preferred here too

* @param string ...$tags Optional cache tags
* @return string
*/
public static function flexible(int $freshSeconds, int $totalSeconds, bool $defer = false, ...$tags): string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a big one, in addition to accept int's and use them as seconds, let's add support for users to pass CarbonInterval as well, so people on the latest version of the frame work can use those nifty new seconds() minutes(), ... helper functions.


$response = $this->responseCache->flexible(
$cacheKey,
[$flexibleTime[0], $flexibleTime[1]],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what inside those indexes by looking here. Extract those to a named variable.

README.md Outdated
// Fresh for 3 minutes, then stale for 15 minutes
use Spatie\ResponseCache\Middlewares\FlexibleCacheResponse;

/* Simple example how to use the flexible method:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This first line can be omitted, hold no value. You can add it as the text above the code block.

README.md Outdated
use Spatie\ResponseCache\Middlewares\FlexibleCacheResponse;

/* Simple example how to use the flexible method:
/* between 0-180 seconds we will always serve fresh data from the cache
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's polish this by making these proper sentences: make these start with a capital, and with a .

README.md Outdated

/* Simple example how to use the flexible method:
/* between 0-180 seconds we will always serve fresh data from the cache
/* after 180 seconds we will serve "old" data and send new data to recalculate to the server
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here also make it more readable by adding a comma. "After 180 seconds, we will..." Also do this for any other comments.

README.md Outdated
/* on the next request if the new data is processed by the server we will serve the new data until
/* then we serve old "stale" data
*/
int $freshSeconds = 180;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't valid PHP remove it.

README.md Outdated
int $staleSeconds = 900;
Route::get('/api/posts', 'PostController@index')
->middleware(CacheResponse::flexible(180, 900));
->middleware(FlexibleCacheResponse::flexible($freshSeconds, $staleSeconds));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you can use name arguments to more clearly convey what the flexible method expects.

When we have support for CarbonInterval, you could also use those new second(), minutes() functions in this example.

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

Successfully merging this pull request may close these issues.

2 participants