node-fetch: restore types (for v2), update version, allow agent: false
#58693
Conversation
…ed as of 3.0.0 by @ImRodry" This reverts commit 4d9b113. node-fetch@3 makes two major changes: they started shipping TS types with the package, and they switched it to being an ESM-only module. The latter means that apps which aren't built in a way that supports ESM (or other npm modules depending on `node-fetch` which don't want to break their non-ESM users) need to continue using v2. In fact, the README for `node-fetch` explicitly states > If you cannot switch to ESM, please use v2 which remains compatible > with CommonJS. Critical bug fixes will continue to be published for > v2. Stats on npmjs.com show that v2 has 43 times more downloads in the past 7 days than v3. I applaud node-fetch's forward-thinking approach to ESM, and am not here to tell them that their choice for v3 is wrong. But practically speaking, v2 is still heavily used and many projects will not be able to upgrade to v3 without breaking their own backwards compatibility promises. So it makes sense to continue to maintain v2-specific node-fetch types (especially as there are specific issues with the published types, as the next commit in this PR shows).
v2.6 is the current (and presumably final) minor release of v2. Apparently these types already included at least one v2.6 feature (`agent` as a function). So, updating the version as suggested at-rule DefinitelyTyped#55533 (comment)
The `agent` option is (after checking to see if it's a function, and if so replacing itself with the return value of calling it) passed directly to `http.request` (or `https.request`) so we should be able to pass any type supported there. Notably, `agent: false` is distinct from `agent: undefined`. A PR making essentially the same change was merged upstream (the upstream repo maintains their own types for v3 but not for v2): node-fetch/node-fetch#1502
|
@glasser Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through. This is a live comment which I will keep updated. This PR touches some part of DefinitelyTyped infrastructure, so a DT maintainer will need to review it. This is rare — did you mean to do this? 9 packages in this PR (and infra files)
Code ReviewsThis PR adds a new definition, so it needs to be reviewed by a DT maintainer before it can be merged. You can test the changes of this PR in the Playground. Status
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 58693,
"author": "glasser",
"headCommitOid": "1ac38913f5fb70f6f7947a58c2f7b9907b594fc6",
"mergeBaseOid": "7467038c60726c43ccfc0f084cd8d5d8f062d234",
"lastPushDate": "2022-02-13T21:13:58.000Z",
"lastActivityDate": "2022-02-14T23:56:44.000Z",
"mergeOfferDate": "2022-02-13T21:27:27.000Z",
"mergeRequestDate": "2022-02-14T23:56:44.000Z",
"mergeRequestUser": "glasser",
"hasMergeConflict": false,
"isFirstContribution": true,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": null,
"kind": "edit",
"files": [
{
"path": "notNeededPackages.json",
"kind": "infrastructure"
}
],
"owners": [],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
},
{
"name": "file-fetch",
"kind": "edit",
"files": [
{
"path": "types/file-fetch/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"tpluscode"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
},
{
"name": "frisby",
"kind": "edit",
"files": [
{
"path": "types/frisby/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"cwoodland",
"johnny4753"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
},
{
"name": "libnpmpublish",
"kind": "edit",
"files": [
{
"path": "types/libnpmpublish/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"nicolas377",
"Guy-Adler"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
},
{
"name": "make-fetch-happen",
"kind": "edit",
"files": [
{
"path": "types/make-fetch-happen/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"abernix",
"trevor-scheer"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
},
{
"name": "node-fetch",
"kind": "add",
"files": [
{
"path": "types/node-fetch/externals.d.ts",
"kind": "definition"
},
{
"path": "types/node-fetch/index.d.ts",
"kind": "definition"
},
{
"path": "types/node-fetch/node-fetch-tests.ts",
"kind": "test"
},
{
"path": "types/node-fetch/package.json",
"kind": "package-meta-ok"
},
{
"path": "types/node-fetch/tsconfig.json",
"kind": "package-meta-ok"
},
{
"path": "types/node-fetch/tslint.json",
"kind": "package-meta-ok"
}
],
"owners": [],
"addedOwners": [
"torstenwerner",
"nikcorg",
"vinaybedre",
"kyranet",
"AndrewLeedham",
"JasonLi914",
"southpolesteve",
"ExE-Boss",
"alexandrusavin",
"OmgImAlexis",
"kbkk",
"glasser"
],
"deletedOwners": [],
"popularityLevel": "Critical"
},
{
"name": "npm-registry-fetch",
"kind": "edit",
"files": [
{
"path": "types/npm-registry-fetch/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"DefinitelyTyped"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
},
{
"name": "proto-fetch",
"kind": "edit",
"files": [
{
"path": "types/proto-fetch/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"tpluscode"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
},
{
"name": "react-native-version-check",
"kind": "edit",
"files": [
{
"path": "types/react-native-version-check/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"vdelacou",
"KrishyV"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
},
{
"name": "zipkin-instrumentation-fetch",
"kind": "edit",
"files": [
{
"path": "types/zipkin-instrumentation-fetch/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"plantain-00"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "nikcorg",
"date": "2022-02-14T10:37:40.000Z",
"isMaintainer": false
},
{
"type": "approved",
"reviewer": "peterblazejewicz",
"date": "2022-02-13T21:26:49.000Z",
"isMaintainer": true
},
{
"type": "stale",
"reviewer": "nicolas377",
"date": "2022-02-12T15:34:58.000Z",
"abbrOid": "af88e00"
},
{
"type": "stale",
"reviewer": "abernix",
"date": "2022-02-11T08:25:43.000Z",
"abbrOid": "af88e00"
},
{
"type": "stale",
"reviewer": "trevor-scheer",
"date": "2022-02-10T01:05:26.000Z",
"abbrOid": "af88e00"
}
],
"mainBotCommentID": 1034320859,
"ciResult": "pass"
} |
typescript-bot
added
New Definition
|
🔔 @tpluscode @cwoodland @johnny4753 @nicolas377 @Guy-Adler @abernix @trevor-scheer @DefinitelyTyped @vdelacou @KrishyV @plantain-00 — please review this PR in the next few days. Be sure to explicitly select |
|
When I run I'm not sure why: this PR restores But your CI doesn't see that problem, so hopefully all is well. |
|
Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files. frisby (unpkg)was missing the following properties:
make-fetch-happen (unpkg)was missing the following properties:
|
typescript-bot
moved this from Waiting for Code Reviews
to Needs Maintainer Action
in New Pull Request Status Board
|
The two packages mentioned by @DangerBotOSS only have dependencies changed in this PR (a revert of the dependency change from #55533). |
There was a problem hiding this comment.
Reviewed changes post-revert.
I don't understand the versioning situation - should what currently exists be 2.6 or do your changes move us to 2.6? I also need to refamiliarize myself with how versioning is handled in DT (mostly, should we be changing the header manually or is that automated in some way?). I'm guessing you did the right thing, only asking because I don't actually know.
|
I changed the version as suggested by a DT maintainer on #55533. The goal I think is to get the major.minor matching the actual package. The types already had support for " |
|
That's my understanding of the goal too. LGTM |
|
By the way, if it's helpful, I'd be happy to be a new maintainer on this project. |
|
@glasser you already are :-) |
Co-authored-by: Nicolas Rodriguez <programnicolas@gmail.com>
|
@nicolas377 I'll take your word for it! I guess something about what needs to be in tsconfig.json changed in the last few months? |
typescript-bot
moved this from Needs Maintainer Action
to Waiting for Code Reviews
in New Pull Request Status Board
typescript-bot
moved this from Waiting for Code Reviews
to Needs Maintainer Action
in New Pull Request Status Board
|
@nicolas377, @abernix, @trevor-scheer Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
typescript-bot
moved this from Needs Maintainer Action
to Waiting for Code Reviews
in New Pull Request Status Board
typescript-bot
moved this from Waiting for Code Reviews
to Needs Maintainer Action
in New Pull Request Status Board
|
@peterblazejewicz, @nikcorg, @nicolas377, @abernix, @trevor-scheer Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review? |
There was a problem hiding this comment.
LGTM!
@glasser thanks!
Please stay put in case something gets broken, thx!
typescript-bot
added
Maintainer Approved
Self Merge
typescript-bot
moved this from Needs Maintainer Action
to Waiting for Author to Merge
in New Pull Request Status Board
|
Cool! I'll do so tomorrow when I'm in more "work mode". |
typescript-bot
added
the
Other Approved
|
Ready to merge |
typescript-bot
moved this from Waiting for Author to Merge
to Recently Merged
in New Pull Request Status Board
|
Thanks everyone! One final question: I see that this published to the |
|
It seems like the v3's just ought to be deprecated (it looks like the most recent |
|
Seems like @types/node-fetch v2.6.0 has broken our latest builds that include @azure/core-http. The latter requires node-fetch@2.6.x, which pulls in the new published types version. However, the published types appear broken because it cannot find the .d.ts file for the
Pinning to the 2.5.x types has resolved the issue for me. |
|
/cc @glasser |
v2.6 depends on v2.3 of `form-data`. That verison of `form-data` is not TS native, requires different dependency DefinitelyTyped#58693 (comment) /cc @glasser @distracteddev Thanks!
|
PR: #58804 |
v2.6 depends on v2.3 of `form-data`. That verison of `form-data` is not TS native, requires different dependency DefinitelyTyped#58693 (comment) /cc @glasser @distracteddev Thanks!
….0.0 by @peterblazejewicz * fix(node-fetch): swap dependency to @types/form-data v2.6 depends on v2.3 of `form-data`. That verison of `form-data` is not TS native, requires different dependency #58693 (comment) /cc @glasser @distracteddev Thanks! * Update types/node-fetch/package.json * Update types/node-fetch/package.json * Update types/node-fetch/package.json Co-authored-by: David Glasser <glasser@apollographql.com> Co-authored-by: David Glasser <glasser@apollographql.com>
…update version, allow `agent: false` by @glasser * Revert "🤖 Merge PR DefinitelyTyped#55533 Remove node-fetch - not needed as of 3.0.0 by @ImRodry" This reverts commit 4d9b113. node-fetch@3 makes two major changes: they started shipping TS types with the package, and they switched it to being an ESM-only module. The latter means that apps which aren't built in a way that supports ESM (or other npm modules depending on `node-fetch` which don't want to break their non-ESM users) need to continue using v2. In fact, the README for `node-fetch` explicitly states > If you cannot switch to ESM, please use v2 which remains compatible > with CommonJS. Critical bug fixes will continue to be published for > v2. Stats on npmjs.com show that v2 has 43 times more downloads in the past 7 days than v3. I applaud node-fetch's forward-thinking approach to ESM, and am not here to tell them that their choice for v3 is wrong. But practically speaking, v2 is still heavily used and many projects will not be able to upgrade to v3 without breaking their own backwards compatibility promises. So it makes sense to continue to maintain v2-specific node-fetch types (especially as there are specific issues with the published types, as the next commit in this PR shows). * Update tslint.json for change from DefinitelyTyped#57489 * node-fetch: update version to 2.6 v2.6 is the current (and presumably final) minor release of v2. Apparently these types already included at least one v2.6 feature (`agent` as a function). So, updating the version as suggested at-rule DefinitelyTyped#55533 (comment) * node-fetch: `agent` can be a boolean The `agent` option is (after checking to see if it's a function, and if so replacing itself with the return value of calling it) passed directly to `http.request` (or `https.request`) so we should be able to pass any type supported there. Notably, `agent: false` is distinct from `agent: undefined`. A PR making essentially the same change was merged upstream (the upstream repo maintains their own types for v3 but not for v2): node-fetch/node-fetch#1502 * Update types/node-fetch/tsconfig.json Co-authored-by: Nicolas Rodriguez <programnicolas@gmail.com> * Update types/node-fetch/index.d.ts Co-authored-by: Niklas Lindgren <nikc@iki.fi> * Update types/node-fetch/package.json Co-authored-by: Piotr Błażejewicz (Peter Blazejewicz) <peterblazejewicz@users.noreply.github.com> Co-authored-by: Nicolas Rodriguez <programnicolas@gmail.com> Co-authored-by: Niklas Lindgren <nikc@iki.fi> Co-authored-by: Piotr Błażejewicz (Peter Blazejewicz) <peterblazejewicz@users.noreply.github.com>
…ependency to ^3.0.0 by @peterblazejewicz * fix(node-fetch): swap dependency to @types/form-data v2.6 depends on v2.3 of `form-data`. That verison of `form-data` is not TS native, requires different dependency DefinitelyTyped#58693 (comment) /cc @glasser @distracteddev Thanks! * Update types/node-fetch/package.json * Update types/node-fetch/package.json * Update types/node-fetch/package.json Co-authored-by: David Glasser <glasser@apollographql.com> Co-authored-by: David Glasser <glasser@apollographql.com>
As discussed with @sandersn on #55533, this PR restores previously-deleted types for
node-fetch@2. It updates the version from v2.5 to v2.6 and fixes an error in the types whereagent: falsewas not allowed.The individual commits in the PR have more detailed commit messages.