chore: remove triple-slash directives from typings (#1285) - #1287
#1287Conversation
|
@yo1dog Does removing |
I removed the line there were no TypeScript errors in index.d.ts. |
|
By the Blame feature (https://github.com/node-fetch/node-fetch/blame/6425e2021a7def096e13dbabcac2f10e6da83d11/%40types/index.d.ts) I found https://github.com/node-fetch/node-fetch/pull/1141/files#r671191147 which already more or less predicted that this What is missing and how can we unblock it? Would it help to perhaps put the typechecking on CI so that we'll not need someone to manually verify that the types are still correct? |
PeterF521
left a comment
There was a problem hiding this comment.
I agree with this change, because adding a reference of DOM library pollutes global scope with useless DOM declarations, considering that node-fetch is only used in Node.js projects.
|
🎉 This PR is included in version 3.2.9 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
I guess my project relied on these definitions, because it now fails with: I think it's now trying to use this as a type: https://github.com/jimmywarting/FormData/blob/0fc449d79874bb4bcea45266080254f451b15a37/esm.min.d.ts#L1-L4 But errors also pop up in dependencies. The simple workaround is adding |
I am unable to replicate. I just did a quick test: Node 16.14.2 No errors. Can you share what version of Node and Typescript you are using? Also the installed versions of node-fetch and formdata-polyfill? |
|
I can reproduce it with your example if I instead do node.js v16.15.0 |
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 ```
What is the purpose of this pull request?
What changes did you make? (provide an overview)
Remove the
domreference triple-slash directive from the typings file which pollutes the global ambient declarations with DOM types.Which issue (if any) does this pull request address?
Is there anything you'd like reviewers to know?