Skip to content
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

bugfix: Fix the types of AbortSignal #1635

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

Conversation

mathieudutour
Copy link

#1287 has the unintended consequence of breaking the types of AbortSignal. This fixes it.

The AbortSignal definition from node-fetch is different from the node's one.

This wasn't an issue before because the dom types were imported and were extending the nodejs types with the event listener methods, but that's not the case anymore.

The issue can be reproduce with the following steps:

mkdir test && cd test
npm i node-fetch @types/node typescript
touch test.ts
touch tsconfig.json

with the tsconfig.json:

{
  "include": ["test.ts"],
  "compilerOptions": {
    "lib": ["es2022"],
    "module": "es2022",
    "target": "es2022",
    "skipLibCheck": true,
    "moduleResolution": "node"
  }
}

and the test.ts (taken from the README):

import fetch, { AbortError } from "node-fetch";

const controller = new AbortController();
const timeout = setTimeout(() => {
  controller.abort();
}, 150);

try {
  const response = await fetch("https://example.com", {
    signal: controller.signal,
  });
  const data = await response.json();
} catch (error) {
  if (error instanceof AbortError) {
    console.log("request was aborted");
  }
} finally {
  clearTimeout(timeout);
}

Running npx tsc --project ./tsconfig.json will show

test.ts:10:5 - error TS2739: Type 'AbortSignal' is missing the following properties from type 'AbortSignal': addEventListener, removeEventListener

10     signal: controller.signal,
       ~~~~~~

  node_modules/node-fetch/@types/index.d.ts:91:2
    91  signal?: AbortSignal | null;
        ~~~~~~
    The expected type comes from property 'signal' which is declared here on type 'RequestInit'


Found 1 error in test.ts:10

node-fetch#1287 has the unintended consequence of breaking the types of `AbortSignal`. This fixes it.

The [`AbortSignal` definition from `node-fetch`](https://github.com/node-fetch/node-fetch/blob/bcfb71c7d10da252280d13818daab6925e12c368/%40types/index.d.ts#L14-L19) is different from the [node's one](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/node/globals.d.ts#L59-L65).

This wasn't an issue before because the dom types were imported and were extending the nodejs types with the event listener methods, but that's not the case anymore.

The issue can be reproduce with the following steps:
```
mkdir test && cd test
npm i node-fetch @types/node typescript
touch test.ts
touch tsconfig.json
```

with the `tsconfig.json`:
```json
{
  "include": ["test.ts"],
  "compilerOptions": {
    "lib": ["es2022"],
    "module": "es2022",
    "target": "es2022",
    "skipLibCheck": true,
    "moduleResolution": "node"
  }
}
```

and the test.ts (taken from the README):
```ts
import fetch, { AbortError } from "node-fetch";

const controller = new AbortController();
const timeout = setTimeout(() => {
  controller.abort();
}, 150);

try {
  const response = await fetch("https://example.com", {
    signal: controller.signal,
  });
  const data = await response.json();
} catch (error) {
  if (error instanceof AbortError) {
    console.log("request was aborted");
  }
} finally {
  clearTimeout(timeout);
}
```

Running `npx tsc --project ./tsconfig.json` will show
```
test.ts:10:5 - error TS2739: Type 'AbortSignal' is missing the following properties from type 'AbortSignal': addEventListener, removeEventListener

10     signal: controller.signal,
       ~~~~~~

  node_modules/node-fetch/@types/index.d.ts:91:2
    91  signal?: AbortSignal | null;
        ~~~~~~
    The expected type comes from property 'signal' which is declared here on type 'RequestInit'


Found 1 error in test.ts:10
```
Copy link

@Crepot Crepot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants