Introduce public fetch method for reading complete responses; introduce new head() method
#110
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
trevor-scheer
changed the title
fetch method for reading complete responses; introduce new head() method
|
After this is merged, we can add something to the larger |
src/__tests__/HTTPCache.test.ts
Outdated
| @@ -489,4 +489,23 @@ describe('HTTPCache', () => { | |||
|
|
|||
| expect(await response.json()).toEqual({ name: 'Ada Lovelace' }); | |||
| }); | |||
|
|
|||
| describe('HEAD requests', () => { | |||
| it('returns a cached GET response when the method is HEAD', async () => { | |||
There was a problem hiding this comment.
is this a good thing? seems strange to me to get something with a body from a HEAD.
There was a problem hiding this comment.
Squashed this behavior via 4cf502e
There was a problem hiding this comment.
why isn't the test failing then?
trevor-scheer
force-pushed
the
trevor/complete-response-fetch-head-method
branch
from
d82e06c
to
7d6cb82
Compare
.changeset/fifty-dots-watch.md
Outdated
| Update default `cacheKeyFor` to include method | ||
|
|
||
| In its previous form, `cacheKeyFor` only used the URL to calculate the cache | ||
| key. As a result, when `cacheOptions.ttl` was specified, the method was ignored. |
There was a problem hiding this comment.
Changesets in this PR are inconsistent as to whether lines are hard-wrapped or not. Not sure what we usually do or if it matters (I think we mostly don't hard-wrap lines in MD files? definitely not in docs?) but I definitely noticed on GH that it's not consistent across the 3 here.
src/__tests__/HTTPCache.test.ts
Outdated
| @@ -489,4 +489,23 @@ describe('HTTPCache', () => { | |||
|
|
|||
| expect(await response.json()).toEqual({ name: 'Ada Lovelace' }); | |||
| }); | |||
|
|
|||
| describe('HEAD requests', () => { | |||
| it('returns a cached GET response when the method is HEAD', async () => { | |||
There was a problem hiding this comment.
why isn't the test failing then?
trevor-scheer
force-pushed
the
trevor/complete-response-fetch-head-method
branch
from
9d55193
to
6817d22
Compare
In its current form, `cacheKeyFor` only uses the URL. As a result, when `cacheOptions.ttl` is specified, the method is ignored. This leads to surprising behavior where a POST request's response is cached and returned for a GET request (for example).
trevor-scheer
force-pushed
the
trevor/complete-response-fetch-head-method
branch
from
998145e
to
3a81baf
Compare
README.md
Outdated
| const cacheKey = this.cacheKeyFor(url, request); | ||
| if (request.method === 'GET') { | ||
| if (['GET', 'HEAD'].includes(request.method)) { |
There was a problem hiding this comment.
I think you need the const method = here too
Introduce a public
fetchmethod which returns a complete response object and the parsed body rather than just the parsed body. This is useful when additional information about the response is needed, such as accessing response headers.With the introduction of the
fetchmethod, providing a newhead()method is now trivial. Previously, ahead()method was useless without access to the response headers (and access to the body is semantically meaningless for aHEADrequest. Unlike the other fetch method methods, thehead()method returns the response object rather than a body.Fixes #27, Fixes #49