fix: TS types support agent: false
#1502
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glasser
changed the title
agent: falseagent: false
The `agent` option is (after being potentially called as as function) passed directly to `http.request`. This is allowed to be a boolean; `http.request` interprets `agent: false` as "allocate a one-off Agent for this request", which is distinct from `agent: undefined` which means to use a default shared agent. This PR updates the TS types to match the existing behavior.
glasser
force-pushed
the
glasser/types-agent-boolean
branch
from
2486582
to
7c8ac61
Compare
jimmywarting
approved these changes
Feb 9, 2022
9 tasks
LinusU
approved these changes
Feb 9, 2022
glasser
added a commit
to glasser/DefinitelyTyped
that referenced
this pull request
Feb 9, 2022
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
typescript-bot
pushed a commit
to DefinitelyTyped/DefinitelyTyped
that referenced
this pull request
Feb 14, 2022
… allow `agent: false` by @glasser * Revert "🤖 Merge PR #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 #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 #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>
martin-badin
pushed a commit
to martin-badin/DefinitelyTyped
that referenced
this pull request
Feb 23, 2022
…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>
|
🎉 This PR is included in version 3.2.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
mook-as
added a commit
to mook-as/rd
that referenced
this pull request
May 31, 2022
node-fetch/node-fetch#1502 changed the type of options.agent to allow a boolean, which caused issues as we tried to call it. Flip the condition around and check for function instead, so that this compiles. Signed-off-by: Mark Yen <mark.yen@suse.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
The
agentoption is (after being potentially called as as function)passed directly to
http.request. This is allowed to bea boolean;
http.requestinterpretsagent: falseas"allocate a one-off Agent for this request", which is
distinct from
agent: undefinedwhich means to use adefault shared agent.
Changes
This PR updates the TS types to match the existing behavior.