fix: cache-control for older browsers

[JoviDeCroock]

Fixes: #2605

In older browsers undefined cache-control means it being cached indefinitely (or I'm doing something wrong but as I see other people are experiencing this as well).

This is a draft PR since I'll have to adjust tests for this, so I first want to run this by you all.

[abernix]

Interesting! What's the definition of older browsers here?

[JoviDeCroock]

Interesting! What's the definition of older browsers here?

In my case this bug report came in from an IE11 and a certain version of Edge

[abernix]

I see! Do you have any other details from the bug report? For example, is there a Last-Modified header present or an Expires header?

Overall though, I think I agree with the idea of ensuring that these are not cached by being more explicit and always providing the max-age=0 when we've decided that it's not cacheable. That said, we should be respectful to anything that does check the truthiness of computeOverallCachePolicy, such as:

apollo-server/packages/apollo-cache-control/src/index.ts

Lines 164 to 173 in d1a0d16

	const overallCachePolicy = this.computeOverallCachePolicy();
	if (overallCachePolicy) {
	o.graphqlResponse.http.headers.set(
	'Cache-Control',
	`max-age=${
	overallCachePolicy.maxAge
	}, ${overallCachePolicy.scope.toLowerCase()}`,
	);
	}

I believe this change would result in that conditional always being truth-y, so we should just adjust that to not check it conditionally if the result is guaranteed.

Additionally, and probably worth considering, @fabsrc recently opened a PR (#2715) which avoided setting Cache-Control under error conditions. I think that having errors not be cacheable makes sense, but perhaps — in a similar way — it would be best to explicitly return max-age=0 for those cases rather than avoiding the cache-control header altogether.

@fabsrc: Do you have thoughts on this?

[fabsrc]

I actually also came across this issue recently when I was configuring a CDN. Apollo not sending a Cache-Control header if the GraphQL response is not cachable meant that I had to set the default cache time to 0.

So I like the idea of being more explicit here and always send a Cache-Control header (if calculateHttpHeaders is set to true). Of course this should then also be sent if the GraphQL response contains errors.

You could also consider using no-cache, no-store, must-revalidate instead of max-age=0 (see https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control).

[jimsorock]

Is this fix not happening? Has it happened elsewhere? Thanks!

Note we ran into the same issue when configuring with CDN.

[JoviDeCroock]

@jimsorock I'm going to close this, my PR section is getting crowded and this does not seem to gain traction

[abernix]

I'm very much in favor of the direction noted in #2675 (comment) if someone is willing and able to pick this up!