New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Docs: Update copy regarding poly/ponyfills #158
base: master
Are you sure you want to change the base?
Conversation
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 about unfetch
/isomorphic-unfetch
! Thanks for contributing.
}); | ||
``` | ||
|
||
Note: we recommend using a version of `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 to isomorphic-unfetch
—not just node-fetch
. We should update this comment to reflect that, i.e. mention the minimum version of isomorphic-unfetch
that supports Brotli compression (uses node-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 the latest)
}); | ||
``` | ||
|
||
Note: we recommend using a version of `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
AbortController
to 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 by node-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';
const unsplash = createApi({
accessKey: 'MY_ACCESS_KEY',
});
const controller = new AbortController();
const signal = 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 a runtime error, but it is different from the one you mentioned:
const controller = new AbortController();
^
ReferenceError: AbortController is not defined
For that, I think we should add a note to the docs that an AbortController
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.
@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.ts
import _fetch, { RequestInit as _RequestInit } from 'node-fetch';
declare global {
export interface RequestInit extends _RequestInit {
cache?: any;
credentials?: any;
integrity?: any;
keepalive?: any;
mode?: any;
referrer?: any;
referrerPolicy?: any;
window?: any;
}
export const fetch: 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-fetch
itself will only work as a ponyfill.If someone were to try to use
node-fetch
directly as recommended in the docs in a TS project, they're more likely to run into a few issues. Depending on thenode-fetch
version 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: