Use custom fetch handler if provided #174
Conversation
There was a problem hiding this comment.
This is great @alanfriedman! Thanks for putting this together. I left a few comments/questions below, but nothing major.
Can you update the docs in this PR to reflect this change? We should update the following sections at least:
src/middleware.js
Outdated
| body, | ||
| headers, | ||
| options = {}, | ||
| fetch: customFetch |
There was a problem hiding this comment.
Nice! For simplicity below (line 51), what if this line read: fetch: doFetch = global.fetch
This way, we can call doFetch internally and it will always be callAPI.fetch, with a default of global.fetch
There was a problem hiding this comment.
Yes, good call!
There was a problem hiding this comment.
Actually we can't always rely on global being defined right?
There was a problem hiding this comment.
Great catch. Yeah, this should be just fetch. This middleware expects fetch to be available globally, whether that's on window in browsers or global in node.
There was a problem hiding this comment.
Updated to default to fetch
| foo: 'bar' | ||
| }), | ||
| { | ||
| // Example of custom `res.ok` |
There was a problem hiding this comment.
#170 is addressing the ability to customize "ok" behavior per-request, what's going on here?
There was a problem hiding this comment.
res.ok is true when the status is 200-299, so this is just an example of setting that boolean with something aside from the actual response status code.
There was a problem hiding this comment.
Gotcha. The issue I linked to is proposing a way to pass in your own 'ok' check instead of res.ok so I was just confused seeing this here. Carry on!
README.md
Outdated
|
|
||
| A custom [Fetch](https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/fetch) implementation, useful for intercepting the fetch request to customize the response status, modify the response payload or skip the request altogether and provide a cached response instead. | ||
|
|
||
| `fetch` must be a function that returns a [Promise](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise) which resolves with an instance of [Request](https://developer.mozilla.org/en-US/docs/Web/API/Response). |
There was a problem hiding this comment.
- This should resolve to Response, yeah?
- Also this only has to conform to the Fetch/Request/Response API, not actually to resolve to a Response object. ie, you can resolve to an object that has
json(),statusCode, etc as this middleware isn't doing any sort of type checking here.
Can I suggest phrasing this as:
A custom Fetch implementation, useful for intercepting the fetch request to customize the response status, modifying the response payload or skipping the request altogether and providing a cached response instead.
If provided, the
fetchoption must be a function that conforms to the Fetch API. Otherwise, the globalfetchwill be used.
There was a problem hiding this comment.
Oops, that was a typo. Also good point, it doesn't need to be a Response object.
You're not wrong, but custom |
|
Released in v2.3.0 |
|
In #171 I added an option so user can add global fetch when creating the middleware |
Just a first pass, opening PR to start a discussion. Let me know what you think. Thanks!
To do: