Skip to content
This repository has been archived by the owner on Aug 15, 2024. It is now read-only.

Docs: Update copy regarding poly/ponyfills #158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

msutkowski
Copy link

Overview

Updates copy around node usage and prefers isomorphic-unfetch, which wraps both of the currently suggested libs. Additionally, this functions as a polyfill, while node-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 the node-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:

message: 'Expected signal to be an instanceof AbortSignal',

@msutkowski msutkowski requested a review from a team as a code owner January 7, 2021 06:38
Copy link
Member

@OliverJAsh OliverJAsh left a 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.
Copy link
Member

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).

Copy link
Author

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.
Copy link
Member

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).

Copy link
Author

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
Copy link
Author

msutkowski commented Jan 7, 2021

@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 cross-fetch or isomorphic-unfetch due to how they export types, but it is possible with node-fetch. Ideally, these libraries would also polyfill the global types as well as the run time, but they do not. In this case, a user could use isomorphic-unfetch and npm i @types/node-fetch and do what's shown above, but it might make sense just to revert back to the original recommendations.

Which way makes the most sense to you?

Edit: as another note, someone could just add 'dom' to lib in their tsconfig, but I'd advise against recommending this as its not accurate and would be a potentially 'broken' experience.

@Magellol Magellol removed their request for review June 2, 2023 13:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants