Switching to node-fetch to avoid caching bugs in make-fetch-happen #2171
Conversation
coreFile by file impact
extrasFile by file impactPull request have no impact on |
joeldenning
changed the title
src/features/node-fetch.js
Outdated
| return fetch(url); | ||
| return fetch(url).then(function (res) { | ||
| // Workaround for https://github.com/npm/make-fetch-happen/issues/28 | ||
| return res.status === 304 ? fetch(url, { cache: 'reload' }) : res; |
There was a problem hiding this comment.
So, this effectively turns every "revalidate" into a double latency request since it will first send the revaldiation request, then immediately send a new request to get a 200. As a result it's double latency by default, which isn't ideal.
In addition, the only remaining cache case with this change is the case of there being no revalidation request - but even then I assume the headers won't be available either? That is we might still have an error case there.
Overall it seems the best approach for now might be just to revert back to node-fetch until this header caching is solved.
| @@ -23,8 +23,6 @@ export function applyImportMap(loader, newMap, mapBase) { | |||
| loader[IMPORT_MAP_PROMISE] = Promise.resolve(); | |||
| } | |||
|
|
|||
| export { clearFetchCache } from './features/node-fetch.js'; | |||
There was a problem hiding this comment.
Removing this is technically a breaking change. However, I seriously doubt people will have already started using this.
If we want to avoid the breaking change, we could make this be a no-op function for now.
|
I have updated this to use node-fetch instead of make-fetch-happen. |
joeldenning
changed the title
I have created npm/make-fetch-happen#28 to see if there's something that can be done within make-fetch-happen to fix this. In the meantime, this workaround seems to be fine. Setting cache to reload will force a normal request and then populate the cache with the new response.