- Notifications
You must be signed in to change notification settings - Fork161
Docs: Update copy regarding poly/ponyfills#158
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
OliverJAsh left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TIL aboutunfetch/isomorphic-unfetch! Thanks for contributing.
| ``` | ||
| Note: we recommend using a versionof`node-fetch`higher than`2.4.0` to benefit from Brotli compression. | ||
| Note:If you choose to use[node-fetch](https://github.com/node-fetch/node-fetch),we recommend using a version higher than`2.4.0` to benefit from Brotli compression. In addition, you may need to import an`AbortController` to patch TypeScript-related issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this note about Brotli compression also applies toisomorphic-unfetch—not justnode-fetch. We should update this comment to reflect that, i.e. mention the minimum version ofisomorphic-unfetch that supports Brotli compression (usesnode-fetch higher than 2.4.0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good point -isomorphic-unfetch should be fine assuming a user is on thelatest)
| ``` | ||
| Note: we recommend using a versionof`node-fetch`higher than`2.4.0` to benefit from Brotli compression. | ||
| Note:If you choose to use[node-fetch](https://github.com/node-fetch/node-fetch),we recommend using a version higher than`2.4.0` to benefit from Brotli compression. In addition, you may need to import an`AbortController` to patch TypeScript-related issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In addition, you may need to import an
AbortControllerto patch TypeScript-related issues.
Is this referring to the other comment you left in the PR description (below)?
Additionally, any version > 3 for a TS user would currently see an error like when making a request:
message: 'Expected signal to be an instanceof AbortSignal',
If so, I'm not sure I understand what the problem is. It looks like the error message you mentioned is a runtime error, thrown bynode-fetch, not a TS (compile time) type error?
Perhaps you could provide a reduced test case of the problem to help me understand?
I tried copying the example from our docs:
import'isomorphic-unfetch';import{createApi}from'unsplash-js';constunsplash=createApi({accessKey:'MY_ACCESS_KEY',});constcontroller=newAbortController();constsignal=controller.signal;unsplash.photos.get({photoId:'123'},{ signal}).catch((err)=>{if(err.name==='AbortError'){console.log('Fetch aborted');}});controller.abort();
Using TS 4 I have no type errors. When I try to run this in Node I do get aruntime error, but it is different from the one you mentioned:
const controller = new AbortController(); ^ReferenceError: AbortController is not definedFor that, I think we should add a note to the docs that anAbortController polyfill/ponyfill is required in Node (separate to the changes in this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sorry about that - I shouldn't work at night. You're correct, it's a run-time error, not a TS error.
The confusing part to this is the current behaviors between node-fetch 2.6 and 3, and I was trying to account for the edge cases. (I tested isomorphic-unfetch, cross-fetch, node-fetch with and without abort controllers)
Thanks again for your review, I'll go ahead and do a cleanup pass on this shortly.
msutkowski commentedJan 7, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@OliverJAsh After a little more thought, I'd also like to document getting things working for TS users in a node env as well. As an example, this kind of thing is necessary: // ./src/global.d.tsimport_fetch,{RequestInitas_RequestInit}from'node-fetch';declare global{exportinterfaceRequestInitextends_RequestInit{cache?:any;credentials?:any;integrity?:any;keepalive?:any;mode?:any;referrer?:any;referrerPolicy?:any;window?:any;}exportconstfetch:typeof_fetch;} You can't do this with Which way makes the most sense to you? Edit: as another note, someone could just add 'dom' to |
Overview
Updates copy around node usage and prefers
isomorphic-unfetch, which wraps both of the currently suggested libs. Additionally, this functions as a polyfill, whilenode-fetchitself will only work as a ponyfill.If someone were to try to use
node-fetchdirectly as recommended in the docs in a TS project, they're more likely to run into a few issues. Depending on thenode-fetchversion used, a user may need to import abort-controller and set it on global. Additionally, any version > 3 for a TS user would currently see an error like when making a request: