🐛[breaking change] cache no-store errors break existing setups

[helloimalastair]

Followup to #2073

The change to support cache, while nice, has introduced some issues with libraries that already used RequestInit.cache, like @tidbcloud/serverless(issue), and @azure/cosmos(discord thread).

Libraries built for the browser/other runtimes, which previously worked because the cache field was just ignored, are now erroring when they attempt to provide no-store, since it isn't yet implemented.

After further investigation, it appears that @tidbcloud/serverless was using new Request('x:', { cache: "no-store" }) in a try/catch to detect whether or not the current runtime supports cache. The new update appears to allow you to create a request with a no-store cache value, but will still error if you try to pass one to fetch().

Would it be possible to have this field log an issue, but not actually throw, so that these libraries continue working? Or, conversely, have new Request() error as it previously did, until no-store is supported?

[jannisch]

Same goes for no-cache, basically any value other than undefined as documented in the code.

[helloimalastair]

Reproduction: Playground

Code:

export default {
	fetch() {
		new Request("https://my.web.site", {
			cache: "no-store" // This should error, OR
		})
		return fetch("https://1.1.1.1/cdn-cgi/trace", {
			cache: "no-store" // This shouldn't error
		});
	}
}

[helloimalastair]

Ok, a little weirder, it does appear that previous versions of wrangler also error, but with a different message. Not sure how @tidbcloud/serverless worked before. Will do a bit more digging...

[kentonv]

Oh man. This is like our worst nightmare for trying to maintain compatibility... apps which dynamically probe for the existence of an API and then attempt to use it if it's there. Code paths that were never tested in any capacity suddenly executing in production due to a runtime change.

Not blaming the application code, but ugh... I guess this means we have to hide this whole API behind a compat flag.

@jasnell

[jannisch]

I totally feel you and that's one thing for sure but the way I understand the PR's description simply does not match the code which also explains the breaking of code only using the RequestInit (@azure/cosmosdb)

[kentonv]

We are going to roll back the runtime to the version before this change went out. This will take a few hours to propagate everywhere.

When we introduce the change again it'll be behind a compat flag.

[jannisch]

Many thanks @kentonv. Similar to what @helloimalastair described, I found that the Azure SDK checks if cache exists in Request.prototype before setting no-store. If you haven't done so already, please look whether you guys can simply ignore no-store and no-cache cache settings in the future.

[kentonv]

@jannisch The plan is to actually support these settings correctly. This change was a first step towards that, but we failed to account for all the ways people might be probing for the existence of the property.

Unfortunately it is apparent that any change at all to the existing behavior risks breaking people -- even if we made the API fully implemented and conformant in a single step, there could still be latent bugs lurking in application code that could kick in as soon as the API becomes available.

So, we will need to guard all changes behind a compat flag. This is unfortunate as we usually don't like making people update their compat date to get access to new APIs but it seems unavoidable here.

[kentonv]

Just to be clear, it is always our fault if code in production breaks as a result of a change we made, not the application's fault. I apologize for the breakage here.

[sumiren]

I am the user who reported the issue on the @tidbcloud/serverless. I confirmed the problem has been resolved. I just wanted to say thank you for your help!

[rishab002]

Is there a workaround ? cache:"no-cache" or "no-store" , is causing this error :The 'cache' field on 'RequestInitializerDict' is not implemented.

[helloimalastair]

@rishab002 That is the original behavior. The workaround is to not set the cache field, or to patch any packages that do so, at least until it is implemented.

[jasnell]

FWIW, our intent here is to properly implement Request.cache for the values no-store, no-cache, and likely reload in the very near future. The change that caused the issue here was a first step to setting that up. I was hoping that because we were just trading one error for another error it wouldn't break things but sadly that is not the case. Our revised plan is to put the entire change behind a compatibility flag and try again.

[jasnell]

Closing this issue. The change was reverted in production.

[tewaro]

@helloimalastair we are about to reroll out this change behind a compatability flag, we added tests for the behavior you described but we'd like to actively monitor your workers as we deploy this change. Could you tell us the zone name of the worker that broke or alternatively email it to [email protected].

[sumiren]

@AdityaAtulTewari
Okay, I will send it to your email.