Use yetch and support AbortController #112
Conversation
|
Just noting some stuff, just doing diligence. yetch: 3,169 (gzipd) Size difference basically doesn't matter. Getting abort control well justifies that. I skimmed the code for I pinged @jayphelps (one of the |
| self['Headers'] = fetch.Headers; | ||
| self['Request'] = fetch.Request; | ||
| self['Response'] = fetch.Response; | ||
| var AbortControllerPolyfill = FastBoot.require('abortcontroller-polyfill/dist/cjs-ponyfill'); |
There was a problem hiding this comment.
Do we need our Fastboot users to also additionally import/require the abortcontroller polyfil, or does this happen automagically? If so, can you briefly explain how?
There was a problem hiding this comment.
Just like with browser implementation :
import fetch, { AbortController } from 'fetch';I will add a paragraph to README to clarify
|
cc @nlfurniss |
|
@tchak have you confirmed all the abort-controller stuff works correctly with node-fetch as well? I don't actually see support for |
|
@stefanpenner @tchak yep node-fetch does not appear to support abort signals yet node-fetch/node-fetch#437 |
|
@tchak I'm unsure if we should go down this path, until node-fetch supports signals. Thoughts? |
|
@stefanpenner regarding node support: https://github.com/mo/abortcontroller-polyfill#using-it-on-nodejs |
|
@tchak ah, thank-you. This lgtm. |
|
released as v4.0.1 🎉 |
|
|
||
| var fetch = normalizeFileName(find(expandedFetchPath)); | ||
| var expandedFetchPath = expand(fetchPath, 'dist/yetch-polyfill.js'); | ||
| var expandedAbortcontrollerPath = expand(abortcontrollerPath, 'abortcontroller-polyfill-only.js'); |
There was a problem hiding this comment.
abortcontroller-polyfill-only.js doesn't have abortableFetch which means we can't do
fetch(Request("http://api.github.com", {signal}))
The test in this PR only tested controller is polyfilled but not abortable fetch.
This is an attempt at #111