Allow `stale-while-revalidate` (and `stale-if-error`?) Cache-Control extensions

[Malvoz]

I believe these directives were dismissed at some point due to no browser support, however there's increasing support for stale-while-revalidate.

stale-if-error on the other hand isn't directly supported by browsers, though some CDNs such as Fastly and KeyCDN support it, see the support table in https://www.ctrl.blog/entry/cdn-rfc5861-support.html.

[antross]

@Malvoz, looking at the code I think both of these are currently allowed. Do you happen to have a test case showing these failing?

Specifically they're listed as extensionDirectives in the code: https://github.com/webhintio/hint/blob/190284e5ff577b6bc0600b1425e7abb39818ced8/packages/hint-http-cache/src/hint.ts#L145

These are included in the check to treat a directive as "valid": https://github.com/webhintio/hint/blob/190284e5ff577b6bc0600b1425e7abb39818ced8/packages/hint-http-cache/src/hint.ts#L192

[Malvoz]

Do you happen to have a test case showing these failing?

Yeah, scanning https://web.dev, using the Chrome extension v2.1.0:

[antross]

Thanks!

Looks like its complaining about the value as opposed to the existence of the directive. This appears to be because we don't account for this in valueDirectives: https://github.com/webhintio/hint/blob/190284e5ff577b6bc0600b1425e7abb39818ced8/packages/hint-http-cache/src/hint.ts#L144

And that causes a failure before we even check for extension directives.

Given the structure of the code, we should probably introduce an extensionValueDirectives list as well and move stale-while-revalidate and stale-if-error there. The current code tries to allow them, but it doesn't work because it treats them as valueless.

[fahensha]

Hi, completely new to open source here but interested in submitting a PR for this issue! Just thought I would drop a comment to make sure the status is still that there needs to be an extensionValueDirectives list and that stale-while-revalidate and stale-if-error need to be moved there and have their values accounted for?

[antross]

Hi @fahensha and thanks for offering to submit a PR!

Yes, your summary is correct. And accounting for the extension values will probably need to happen either before this check or as part of it: https://github.com/webhintio/hint/blob/190284e5ff577b6bc0600b1425e7abb39818ced8/packages/hint-http-cache/src/hint.ts#L165

Feel free to ask questions if you run into any trouble and thanks again!

[fahensha]

While recreating the issue I am running into a problem creating the sideload chrome extension.

Since this is just a set up issue, is it okay to discuss it here, or is there somewhere else I can get help resolving this? Thanks in advance!

[molant]

Here is fine :)

[fahensha]

Okie dokie, when I try to sideload the extension to test changes on my local copy through the "Load Unpacked" option on chrome://extensions/, I get this error message:

File ~/git/hint/packages/extension-browser/dist/bundle Error Required value 'version' is missing or invalid. It must be between 1-4 dot-separated integers each between 0 and 65536. Could not load manifest.

Looking at the manifest the line with the version on it is: "version": "(set by scripts/copy-version.js)" Should I just choose my own version here, or should I run the copy-version.js script, or maybe something else was not happening correctly when I ran yarn build?

Thanks in advance!

[molant]

Sorry for the delay answering 😞 Don't know why the version is not being copied correctly and why we haven't catch this earlier. You should be able to put whatever you like in there or use build-release or build-release:chromium (although those won't be unpacked).

@antross, @sarvaje this PR seems related: https://github.com/webhintio/hint/pull/2964 but don't know why it would have this side effect.

[fahensha]

No worries! I added in my own version number of "1" and now its complaining about not being able to find background-script.js:

Failed to load extension File ~/git/hint/packages/extension-browser/dist/bundle Error Could not load background script 'background-script.js'. Could not load manifest.

[antross]

@fahensha, this definitely sounds like some of our build/bundle steps aren't running correctly.

If you're on a branch with some changes already, can you open a draft PR so we can checkout the same code and see if we also run into this issue? I tried reproducing the issue on main but things appear to be working fine for me.

[fahensha]

@antross Yep sure thing!

[sarvaje]

Hi @fahensha. I have been trying your PR in windows, WSL2 and Ubuntu and I didn't have any problem building or loading the extension.

This is what I did:

1. Clone the repo
2. Checkout your PR
3. Run yarn
4. Run yarn build
5. Go to the browser and load the extension

Can you try cloning your repo in another folder and run those steps?

[fahensha]

Hi @sarvaje ! Trying these steps all again, sorry for the massive delay. I'll let you know how it goes

[fahensha]

I'm not sure what it was that was broken, but re-running these steps did the trick and sideloading the extension works! I think I just had to mess with the build process and make sure I ran yarn build in the root as well as in /packages/extension-browser.

I think the originally given example of https://web.dev with the stale-while-revalidate error is moot because I can't replicate the error both with or without the fix. These are the only warnings for the site for HTTP cache:

Should I just look around for another website that fails this test? Since these are only experimental directives I'm guessing it's pretty unlikely that I'd just stumble across one :/ Thanks for your help and your patience!

[antross]

@fahensha looks like it's our turn to apologize for the delay over the holidays. Sorry about that.

I don't currently know of a live URL with this issue, though we just had an issue report from someone else who apparently has one (I just linked the duped bug #4281). I'm hoping they might be able to share the URL so you can use it to validate the fix.

In the meantime feel free to open a PR with your progress anyway. If all else fails we should be able to help put together a test page we can host locally to verify.

[Simbiat]

I've encountered this during setup on a test environment. If everything goes as planned I may be able to promote it to PROD this weekend and will share link. Even if it's not this weekend, I will share the link as soon as it's live.

[Simbiat]

I was able to promote to PROD a small portion, which utilizes the header in question: https://simbiat.ru/api/atom/bicchanged/ cache-control: max-age=3600, stale-while-revalidate=1800, stale-if-error=1800, no-transform Please, note, that there seems to be a bug on my side, that if 304 is sent, the header cache-control: no-store, no-cache, must-revalidate is sent. I am working on fixing that, but if you're getting it - just force refresh the page

EDIT: Because f data quality issues the above link had to be disabled temporarily, but you can use any other page on https://simbiat.ru

[antross]

Thanks @Simbiat!

@fahensha do you think you'll have an opportunity to verify your changes against this site and post a PR?

[fahensha]

Yeah absolutely :) I would have never thought to just custom make a URL with the specific directive, but that’s a great way to do it! Thanks @Simbiat

[Simbiat]

It's no longer a "custom" page: whole site now should be providing these headers. I am specifically coding some functions in https://github.com/Simbiat/HTTP20 to allow easier standardization of headers on a website. It's work in progress, but one can only hope, that some day it will be used all over and make the web more secure...

[fahensha]

@Simbiat When I first read your message I just assumed that the URL you sent was using the cache-control directives in question, but now I see that its a link to your project which creates standardized functions that helps developers follow best caching practices when designing a website.

I can see that the Headers class with the cacheControl function with cacheStrat parameter 'month' would get me the cache-control directives I need to verify my changes, but I'm not sure how to turn this information into a URL that I can test against. Would you be able to give some instruction on how to use your functions to create a URL I can use?

Thanks in advance!

[Simbiat]

@fahensha actually my https://simbiat.ru is already providing the necessary headers (you can use whatever page there). As for creating a page... I guess I'll add something to readme, but in general, if you want to use my HTTP20 class, I'd recommend using

(new \http20\Common)->zEcho($output, $cacheStrat);

You put whatever HTML into $output and select an optional $cacheStrat ('month' as you have mentioned) and... Enjoy the page you've created :D

[fahensha]

Awesome thank you! This is consistent with discussion above. As @antross put it, "its complaining about the value as opposed to the existence of the directive. This appears to be because we don't account for this in valueDirectives..." in https://github.com/webhintio/hint/blob/190284e5ff577b6bc0600b1425e7abb39818ced8/packages/hint-http-cache/src/hint.ts#L144

[fahensha]

As I'm about to submit a PR with the fix, I am wondering if simply removing the error and accepting the directives as valid is actually the best way to go? @antross

MDN still has the "Experimental. Expect behavior to change in the future," tag on them, and browser support is still lacking, particularly for stalie-if-error source: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control

It seems we have come full circle to the initial discussion, and I wanted to consider this before I make the fix! Maybe changing the error to a warning that is similar to the MDN warning would be a better solution?

[Simbiat]

stale-while-revalidate got adopted by Chrome and Firefox, so that should definitely be supported now.
stale-if-error while not supported, yet, should not be "breaking" anything as well. I mean, if a browser starts supporting it - it will use it. If it does not support it - it will ignore it. So no need to alert about it at this point. If it will be deprecated, then an alert can be added, but its wording will need to be different.

[ashutosh887]

@Malvoz I would like to work on this

[Tristan971]

This issue is still present; reading the associated PR #4047 I checked MDN and I see that:

1. Every browser but Safari supports it
2. It is useful for mid-tier caches (as in proxy caches, to not delay queries while the mid-tier caches fetches from the origin) anyway
3. Even on browsers that don't support it, it doesn't cause any harm either

Given this, I fail to see a good reason to not include it