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

Remove node-fetch - not needed as of 3.0.0 #55533

Merged
merged 2 commits into from Sep 3, 2021
Merged

Remove node-fetch - not needed as of 3.0.0 #55533

merged 2 commits into from Sep 3, 2021

Conversation

ImRodry
Copy link
Contributor

@ImRodry ImRodry commented Sep 1, 2021

Please fill in this template.

If removing a declaration:

  • If a package was never on Definitely Typed, you don't need to do anything. (If you wrote a package and provided types, you don't need to register it with us.)
  • Delete the package's directory.
  • Add it to notNeededPackages.json.

Closes node-fetch/node-fetch#1259

@typescript-bot
Copy link
Contributor

typescript-bot commented Sep 1, 2021

@ImRodry Thank you for submitting this PR!

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?

10 packages in this PR (and infra files)

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes of this PR in the Playground.

Status

  • ✅ No merge conflicts
  • ✅ Continuous integration tests have passed
  • ✅ A DT maintainer needs to approve changes which affect DT infrastructure (notNeededPackages.json)

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": 55533,
  "author": "ImRodry",
  "headCommitOid": "2442a8fed6e7f6569ae4ccd0ba7d03d325c98931",
  "lastPushDate": "2021-09-01T22:09:52.000Z",
  "lastActivityDate": "2021-09-03T15:24:51.000Z",
  "mergeOfferDate": "2021-09-03T15:24:13.000Z",
  "mergeRequestDate": "2021-09-03T15:24:51.000Z",
  "mergeRequestUser": "ImRodry",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": null,
      "kind": "edit",
      "files": [
        {
          "path": "notNeededPackages.json",
          "kind": "infrastructure"
        }
      ],
      "owners": [],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    },
    {
      "name": "adobe__node-fetch-retry",
      "kind": "edit",
      "files": [
        {
          "path": "types/adobe__node-fetch-retry/package.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "ricardoatsouza",
        "joachimroeleveld"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "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": "delete",
      "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/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/node-fetch/tslint.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "torstenwerner",
        "nikcorg",
        "vinaybedre",
        "kyranet",
        "AndrewLeedham",
        "JasonLi914",
        "southpolesteve",
        "ExE-Boss",
        "alexandrusavin",
        "OmgImAlexis",
        "kbkk"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    },
    {
      "name": "npm-registry-fetch",
      "kind": "edit",
      "files": [
        {
          "path": "types/npm-registry-fetch/package.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "ChaosinaCan"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "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": "orta",
      "date": "2021-09-03T15:23:27.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 910679400,
  "ciResult": "pass"
}

@typescript-bot
Copy link
Contributor

🔔 @ricardoatsouza @joachimroeleveld @tpluscode @cwoodland @johnny4753 @nicolas377 @Guy-Adler @abernix @trevor-scheer @torstenwerner @nikcorg @vinaybedre @kyranet @AndrewLeedham @JasonLi914 @southpolesteve @ExE-Boss @alexandrusavin @OmgImAlexis @kbkk @ChaosinaCan @vdelacou @KrishyV @plantain-00 — please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

@typescript-bot typescript-bot removed the Where is GH Actions? GH Actions didn't give a response to this PR label Sep 1, 2021
@typescript-bot typescript-bot moved this from Other to Waiting for Code Reviews in New Pull Request Status Board Sep 1, 2021
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Action in New Pull Request Status Board Sep 1, 2021
@peterblazejewicz
Copy link
Member

peterblazejewicz commented Sep 1, 2021

I"m not sure if simple replacement to v3 as direct dependency if the best approach. It can install dependency and types in a verson which was not tested nor used with original package. Take file-fetch as example, this package depends on v2 of node-fetch, not v3:
https://github.com/bergos/file-fetch/blob/master/package.json#L39
If there are dendencies, one can also use @types/node-fetch@.. (last known version on DT), instead of allowing silent dependency changes. @ImRodry

@ImRodry
Copy link
Contributor Author

ImRodry commented Sep 1, 2021

I"m not sure if simple replacement to v3 as direct dependency if the best approach. It can install dependency and types in a verson which was not tested nor used with original package. Take file-fetch as example, this package depends on v2 of node-fetch, not v3:
https://github.com/bergos/file-fetch/blob/master/package.json#L39
If there are dendencies, one can also use @types/node-fetch@.. (last known version on DT), instead of allowing silent dependency changes. @ImRodry

But all node-fetch dependencies are set to 3.0.0 and I tested them and got no errors. The ones that did error out or used things from the package that are different in node-fetch v3 I kept the types package. Wouldn’t this be alright @peterblazejewicz?

@peterblazejewicz
Copy link
Member

what do you mean by 'tested'? I'd say this when run transpiled native JS. Sorry I'm over-cautious, as I have recently to fix DT package detail due to impact done on native JS code usage in the wild. I'd avoid doing major version bump, unless we've got approvals from all owners for each involved DT package

@ImRodry
Copy link
Contributor Author

ImRodry commented Sep 1, 2021

what do you mean by 'tested'? I'd say this when run transpiled native JS. Sorry I'm over-cautious, as I have recently to fix DT package detail due to impact done on native JS code usage in the wild. I'd avoid doing major version bump, unless we've got approvals from all owners for each involved DT package

The node-fetch package was mostly copied over from here with the 2 stupid changes they noted in their changelog. I understand your concerns though and will lower them all down to @types/node-fetch just to be sure, but I believe it wouldn't make much of a difference

@ImRodry
Copy link
Contributor Author

ImRodry commented Sep 1, 2021

@peterblazejewicz addressed your concerns, let me know if it's all good now!

@typescript-bot typescript-bot added the Check Config Changes a module config files label Sep 1, 2021
@typescript-bot typescript-bot moved this from Needs Maintainer Action to Waiting for Code Reviews in New Pull Request Status Board Sep 1, 2021
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Sep 1, 2021
@typescript-bot
Copy link
Contributor

@ImRodry The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review.

@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Sep 1, 2021
@typescript-bot typescript-bot removed The CI failed When GH Actions fails Check Config Changes a module config files labels Sep 1, 2021
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Sep 1, 2021
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Maintainer Action in New Pull Request Status Board Sep 1, 2021
@southpolesteve
Copy link
Contributor

I'd like to hear to one of the DT team to understand this change better and the impact . Plenty of libraries and projects still depend on node-fetch v2 and these types. Wouldn't it make more sense to release a major version of @types/node-fetch and mark these types as deprecated on NPM? I am sure this is not the first @types library that got superseded by the actual library deciding to include types.

@ImRodry
Copy link
Contributor Author

ImRodry commented Sep 1, 2021

I'd like to hear to one of the DT team to understand this change better and the impact . Plenty of libraries and projects still depend on node-fetch v2 and these types. Wouldn't it make more sense to release a major version of @types/node-fetch and mark these types as deprecated on NPM? I am sure this is not the first @types library that got superseded by the actual library deciding to include types.

That is exactly what this PR is doing. It will release @types/node-fetch v3.0.0 as deprecated and all the packages that depend on it will use the current version. I've done this for mongodb at #54510 and it has also been done for mongoose at #53417

@nicolas377
Copy link
Contributor

I'll update libnpmpublish myself, but we shouldn't be deleting node-fetch's types just yet, just deprecating the package and giving a window of backwards compatibility before deleting the types package

@ImRodry
Copy link
Contributor Author

ImRodry commented Sep 2, 2021

@nicolas377 the node-fetch devs said they would only be providing bug fixes to v2 of the package so we will not need to update any types anymore so there’s no reason not to delete. Of course people will always be able to install the types package with a deprecation warning since that’s how deleting packages works here

@typescript-bot
Copy link
Contributor

I just published @types/proto-fetch@1.0.1 to npm.

@typescript-bot
Copy link
Contributor

I just published @types/react-native-version-check@3.4.3 to npm.

@typescript-bot
Copy link
Contributor

I just published @types/zipkin-instrumentation-fetch@0.11.6 to npm.

@nikcorg
Copy link
Contributor

nikcorg commented Sep 3, 2021

Bye bye @types/node-fetch 👋

I enjoyed being part of the maintainers crew. However, I won't shed a tear at your departure.

@trygve-aaberge-adsk
Copy link

node-fetch is still releasing 2.x versions (2.6.7 was released 12 days ago) which don't have types, so I don't think this should have been deleted.

@tpluscode
Copy link
Contributor

The @types/node-fetch@2 packages still exist. Do they require any changes?

@trygve-aaberge-adsk
Copy link

trygve-aaberge-adsk commented Jan 28, 2022

Oh, the 2.5.12 version of @types/node-fetch actually includes the type changes that was introduced in node-fetch 2.6.0, that was a bit confusing.

E.g. setting options.agent to a function was added in node-fetch 2.6.0: node-fetch/node-fetch#632, but it was added to the types here without bumping the version: #36057 and #36152

And I don't know if it's common to do here, but it would be easier to know that you have the correct types if there was a release of these types for every release of node-fetch, even if there are no type changes. I understand that would increase the maintenance burden though.

@tpluscode
Copy link
Contributor

Yes, the @types/* packages are independently versioned. Typically the major matches but there are multiple projects which are version 1, 2 or 3 and the types are still 0.0.x. I also found this a little confusing at first but think of it this way: package major version indicates breaking changes in functionality. Types major version indicates breaking changes in JS API. The two are not the same thing

@trygve-aaberge-adsk
Copy link

trygve-aaberge-adsk commented Jan 28, 2022

But I thought at least the version comment in the type file should match the package version? The last version of these types still had it set to 2.5. How are you supposed to know that these types are not compatible if you use node-fetch 2.5, and that it includes the new changes if you use node-fetch 2.6?

My understanding up until now was that the major and minor version should match, and that only the patch version doesn't need to match.

@trygve-aaberge-adsk
Copy link

Though, I guess this is really more related to the change being made without bumping the version number than the types being deleted.

@nikcorg
Copy link
Contributor

nikcorg commented Jan 28, 2022

You're right, the version number in the comment not being updated will have been an oversight.

@glasser
Copy link
Contributor

glasser commented Feb 8, 2022

Would it be reasonable to add these back? node-fetch is a very popular library, and they made the choice in v3 to only publish the package as an ESM module, so anyone in a build system that doesn't support ESM (or who is publishing an npm package depending on node-fetch that they'd like folks using non-ESM build systems to be able to consume) is likely to be stuck on v2 for quite a while.

I'd specifically love one improvement. The Request.agent field is passed through directly to Node's http.request function as the agent argument there. boolean is a valid type for this option in Node http.request but the node-fetch types don't allow it. (Passing agent: false is different from agent: undefined; the former means to automatically create an Agent that isn't shared with any other request, whereas the latter means to use the default global shared agent.) Ordinarily I'd send a PR to add that one change to the DefinitelyTyped package but since it's been deleted, I can't.

@tpluscode
Copy link
Contributor

I think you should be asking on https://github.com/node-fetch/node-fetch

This repository only manages typ declarations for versions <3

@glasser
Copy link
Contributor

glasser commented Feb 9, 2022

Yes, that's my point exactly: we are unable to upgrade to v3 so would like to be able to improve the v2 types, which were in this repo only but have been removed.

@nikcorg
Copy link
Contributor

nikcorg commented Feb 9, 2022

I would suggest opening a PR with your proposed changes might be the quickest way forward. It might garner more attention than discussion on an already merged PR. But this is merely speculation on my part. Or at least maybe explicitly mention some of the admins to highlight the issue in their notifications.

Other paths of higher resistance might be to switch clients, as native Fetch API just landed in Node.js. The implementation is at https://github.com/nodejs/undici/

@sandersn
Copy link
Contributor

sandersn commented Feb 9, 2022

From a DT maintainer: sounds like re-adding the node-fetch@2 types is the right way to go. Remember to remove node-fetch from notNeededPackages.json in the DT root.

I didn't quite get all the back-and-forth about 2.5 vs 2.6, but typically the major.minor matches the source package's major.minor, and DT uses the patch version as an incrementing counter for each change to the types. If the DT types are for node-fetch@2.6, I'd recommend publishing them as @types/node-fetch@2.6.

@glasser
Copy link
Contributor

glasser commented Feb 9, 2022

@sandersn Sounds reasonable! I have the start of a "revert this PR and also make the agent fix I want" PR together; I'll file it once upstream merges node-fetch/node-fetch#1502 so it's clear that the second commit matches what the v3 types will be to. I'll try to update the version to v2.6.0 too.

@glasser
Copy link
Contributor

glasser commented Feb 9, 2022

(I assume I should be fully reverting this PR including all the things that add explicit dependencies on @types/node-fetch@2 to other packages in the repo? Or are those still good?)

@peterblazejewicz
Copy link
Member

I"d say only for package that depends on 'node-fetch@2'. This is also sometimes loosely implemented on DT, so take precautions

@glasser
Copy link
Contributor

glasser commented Feb 9, 2022

@peterblazejewicz Sorry I'm not sure if I follow. Should I remove the explicit dependencies on @types/node-fetch added to various packages in this PR when I revert, or should I leave them in place?

glasser added a commit to glasser/DefinitelyTyped that referenced this pull request Feb 9, 2022
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)
@glasser
Copy link
Contributor

glasser commented Feb 9, 2022

Thanks, I filed #58693. I did not understand @peterblazejewicz 's comment so I hope it meant that I should revert the dependency changes from this PR.

typescript-bot pushed a commit 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

V3 followup: update DefinitelyTyped