Skip to content

Commit

Permalink
HTTPCache: FetcherResponse.url should exist consistently (#105)
Browse files Browse the repository at this point in the history
Previously, the `url` on the `FetcherResponse` returned from an
HTTPCache would be set properly if this was an actual fetch, but set to
'' if it were read from the cache.

This issue was introduced in
apollographql/apollo-server#1362 (as part of the
initial creation of RESTDataSource) as part of allowing for custom cache
keys.

Fixes #35.
  • Loading branch information
glasser authored Dec 5, 2022
1 parent f77a22e commit 8af22fe
Show file tree
Hide file tree
Showing 3 changed files with 13 additions and 3 deletions.
5 changes: 5 additions & 0 deletions .changeset/flat-roses-draw.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@apollo/datasource-rest': patch
---

The fetch Response now consistently has a non-empty `url` property; previously, `url` was an empty string if the response was read from the HTTP cache.
7 changes: 5 additions & 2 deletions src/HTTPCache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,10 @@ export class HTTPCache {
const { policy: policyRaw, ttlOverride, body } = JSON.parse(entry);

const policy = CachePolicy.fromObject(policyRaw);
// Remove url from the policy, because otherwise it would never match a request with a custom cache key
// Remove url from the policy, because otherwise it would never match a
// request with a custom cache key (ie, we want users to be able to tell us
// that two requests should be treated as the same even if the URL differs).
const urlFromPolicy = policy._url;
policy._url = undefined;

if (
Expand All @@ -79,7 +82,7 @@ export class HTTPCache {
) {
const headers = policy.responseHeaders();
return new Response(body, {
url: policy._url,
url: urlFromPolicy,
status: policy._status,
headers,
});
Expand Down
4 changes: 3 additions & 1 deletion src/__tests__/HTTPCache.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,14 @@ describe('HTTPCache', () => {
it('returns a cached response when not expired', async () => {
mockGetAdaLovelace({ 'cache-control': 'max-age=30' });

await httpCache.fetch(adaUrl);
const firstResponse = await httpCache.fetch(adaUrl);
expect(firstResponse.url).toBe(adaUrl.toString());

jest.advanceTimersByTime(10000);

const response = await httpCache.fetch(adaUrl);

expect(response.url).toBe(adaUrl.toString());
expect(await response.json()).toEqual({ name: 'Ada Lovelace' });
expect(response.headers.get('age')).toEqual('10');
});
Expand Down

0 comments on commit 8af22fe

Please sign in to comment.