StrategyHandler.getCacheKey() should take the request.url into account

[jeffposnick]

Library Affected:
workbox-strategies

There's currently some aggressive in-memory caching done inside of the getCacheKey() method of the StrategyHandler class:

workbox/packages/workbox-strategies/src/StrategyHandler.ts

Lines 436 to 458 in 4d39dfa

	async getCacheKey(
	request: Request,
	mode: 'read' | 'write',
	): Promise<Request> {
	if (!this._cacheKeys[mode]) {
	let effectiveRequest = request;
	for (const callback of this.iterateCallbacks('cacheKeyWillBeUsed')) {
	effectiveRequest = toRequest(
	await callback({
	mode,
	request: effectiveRequest,
	event: this.event,
	// params has a type any can't change right now.
	params: this.params, // eslint-disable-line
	}),
	);
	}
	this._cacheKeys[mode] = effectiveRequest;
	}
	return this._cacheKeys[mode]!;
	}

If this._cacheKeys[mode] is set (where mode is either 'read' or 'write') then the first value it's set to is reused indefinitely, regardless of the value of the request parameter.

This can cause problems if you want to pass in multiple URLs to the same StrategyHandler instance inside of a custom Strategy subclass. This would come up if you, for instance, have a Strategy class that responded to a request for a URL by creating multiple sub-requests, each of whose responses need to be cached independently. As it stands, those sub-requests will each result in the same cache key being used, effectively overwriting the previous response data with a response that doesn't correspond to the URL.

To fix this, we can use a compound key of ${request} | ${mode} for the in-memory cache. Longer-term, we could consider dropping this in-memory cache entirely, but I realize that changing that behavior might require a major semver release.

CC: @philipwalton in case there was a specific motivation for needing this in-memory cache when it was originally implemented.