@43081j 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?

8 packages in this PR (and infra files)

• got — on npm, on unpkg
• keyv (probably deleted!) — on npm, on unpkg
• keyv__mongo (probably deleted!) — on npm, on unpkg
• keyv__mysql (probably deleted!) — on npm, on unpkg
• keyv__postgres (probably deleted!) — on npm, on unpkg
• keyv__redis (probably deleted!) — on npm, on unpkg
• keyv__sqlite (probably deleted!) — on npm, on unpkg
• onionoo — on npm, on unpkg
• Infra files
  • notNeededPackages.json

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.

{
  "type": "info",
  "now": "-",
  "pr_number": 62741,
  "author": "43081j",
  "headCommitOid": "b842005e001527b23e36de11c6fce4442ee5ba4c",
  "mergeBaseOid": "1ddcc5a0bf1802378a0a17b28b0eba2d9adbde99",
  "lastPushDate": "2022-10-18T17:58:49.000Z",
  "lastActivityDate": "2022-10-18T20:54:47.000Z",
  "mergeOfferDate": "2022-10-18T19:13:06.000Z",
  "mergeRequestDate": "2022-10-18T20:54:47.000Z",
  "mergeRequestUser": "43081j",
  "hasMergeConflict": false,
  "isFirstContribution": false,
  "tooManyFiles": false,
  "hugeChange": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": null,
      "kind": "edit",
      "files": [
        {
          "path": "notNeededPackages.json",
          "kind": "infrastructure"
        }
      ],
      "owners": [],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    },
    {
      "name": "got",
      "kind": "edit",
      "files": [
        {
          "path": "types/got/got-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/got/v8/got-tests.ts",
          "kind": "test"
        }
      ],
      "owners": [
        "BendingBender",
        "LinusU",
        "ikokostya",
        "stijnvn",
        "wingsbob",
        "ryanwilsonperkin",
        "phawxby",
        "ivywit",
        "Huachao"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Popular"
    },
    {
      "name": "keyv",
      "kind": "delete",
      "files": [
        {
          "path": "types/keyv/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/keyv/keyv-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/keyv/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/keyv/tslint.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "Arylo",
        "BendingBender"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    },
    {
      "name": "keyv__mongo",
      "kind": "delete",
      "files": [
        {
          "path": "types/keyv__mongo/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/keyv__mongo/keyv__mongo-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/keyv__mongo/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/keyv__mongo/tslint.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "BendingBender"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "keyv__mysql",
      "kind": "delete",
      "files": [
        {
          "path": "types/keyv__mysql/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/keyv__mysql/keyv__mysql-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/keyv__mysql/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/keyv__mysql/tslint.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "BendingBender"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "keyv__postgres",
      "kind": "delete",
      "files": [
        {
          "path": "types/keyv__postgres/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/keyv__postgres/keyv__postgres-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/keyv__postgres/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/keyv__postgres/tslint.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "BendingBender"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "keyv__redis",
      "kind": "delete",
      "files": [
        {
          "path": "types/keyv__redis/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/keyv__redis/keyv__redis-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/keyv__redis/package.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/keyv__redis/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/keyv__redis/tslint.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "BendingBender"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "keyv__sqlite",
      "kind": "delete",
      "files": [
        {
          "path": "types/keyv__sqlite/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/keyv__sqlite/keyv__sqlite-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/keyv__sqlite/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/keyv__sqlite/tslint.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "BendingBender"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    },
    {
      "name": "onionoo",
      "kind": "edit",
      "files": [
        {
          "path": "types/onionoo/onionoo-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/onionoo/tsconfig.json",
          "kind": "package-meta-ok"
        }
      ],
      "owners": [
        "BendingBender"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Well-liked by everyone"
    }
  ],
  "reviews": [
    {
      "type": "approved",
      "reviewer": "andrewbranch",
      "date": "2022-10-18T19:12:06.000Z",
      "isMaintainer": true
    }
  ],
  "mainBotCommentID": 1279799846,
  "ciResult": "pass"
}

🔔 @BendingBender @LinusU @ikokostya @stijnvn @wingsbob @ryanwilsonperkin @phawxby @ivywit @Huachao @Arylo — 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.

@43081j 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.

Will fix ci soon, my bad 👍

@43081j 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.

Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files.
The check for missing properties isn't always right, so take this list as advice, not a requirement.

got (unpkg)

was missing the following properties:

1. create
2. mergeInstances
3. defaults

Generated by 🚫 dangerJS against b842005

i specified "since 4.5.0" just because its been this way for a while and im not sure which exact version introduced it

unpkg makes it pretty easy to find out. It was 4.2.0:

https://unpkg.com/browse/[email protected]/src/

nice, didn't think of that. have updated the versions now

ready to merge

Looks like this causes some issues - #62793

Could this be reverted/fixed by empty .d.ts file, maybe?

@43081j what was the reason for removing index.d.ts file altogether and not leaving it empty? Did this cause some sort of types collision / shadowing?

that's how this repo works. once the package ships its own types, we remove it entirely and note which version started shipping the types directly.

the fix here is likely to update the consuming package's types to use the DT types rather than keyv directly. its probably because whatever package is consuming it doesn't have keyv available as a dependency somehow

@andrewbranch might know more

100% shouldn't be reverting anything or introducing keyv again.

@Ansile can you be a bit more specific, which package do you use which depends on keyv? the error suggests you have an older keyv dependency, but somehow new types.

that's how this repo works. once the package ships its own types, we remove it entirely and note which version started shipping the types directly.

Ok, then what was the reason behind not making this a major change? This is definitely breaking in nature, and if it was a major version, it wouldn't cause this problem

@Ansile can you be a bit more specific, which package do you use which depends on keyv? the error suggests you don't have keyv as a dependency, why would that be?

@43081j , i do have keyv as a (transient) dependency. The source of the error is missing @types/keyv/index.d.ts file. The reason for the error is described here - microsoft/TypeScript#27956

the fix here is likely to update the consuming package's types to use the DT types rather than keyv directly. its probably because whatever package is consuming it doesn't have keyv available as a dependency somehow

As @types/keyv is a transient dependency in my case (i depend on a package that depends on it), i have no control over whether it's installed. That kind of thing traditionally is supposed to be handled by semver, which i do not think was handled correctly in this case. The keyv package itself is installed though.

@Ansile i don't control the versioning when removing a package, the DT tooling decides that so afraid i can't answer that

may be an issue we need to raise in the dt tools repo.

i suspect the issue you've referenced is a red herring. i think its more likely somewhere in your dependency tree, you have types which depend on keyv. those are expecting you to have @types/keyv available (which you no longer do), so they try find the types in keyv itself and fail because its an older version which didn't ship types.

it'd be helpful if we could know what that dependency is. somewhere it sounds like you have a dependency which depends on keyv.

also, importantly, if you don't choose to upgrade @types/keyv, everything works fine at least right? we haven't broken existing code, just case where someone upgrades. sure there's something to solve here still but just want to confirm that

@Ansile i don't control the versioning when removing a package, the DT tooling decides that so afraid i can't answer that

may be an issue we need to raise in the dt tools repo.

Hm, i did create the issue - #62793

Maybe it's a bit of an unhandled case, or maybe the issue with missing index.d.ts files is not documented well enough to be considered when writing something like that by typescript team.

i suspect the issue you've referenced is a red herring. i think its more likely somewhere in your dependency tree, you have types which depend on keyv. those are expecting you to have @types/keyv available (which you no longer do), so they try find the types in keyv itself and fail because its an older version which didn't ship types.

@43081j, i do have @types/keyv installed though.

The case behind the error is exactly it being installed but not having index.d.ts file

it'd be helpful if we could know what that dependency is. somewhere it sounds like you have a dependency which depends on keyv.

That would be @types/[email protected]

It looks like it's also been removed from definitelyTyped, but it's previous versions depend on "@types/keyv": "*", which actually means that major version would not have helped. That's pretty fragile :(

also, importantly, if you don't choose to upgrade @types/keyv, everything works fine at least right? we haven't broken existing code, just case where someone upgrades. sure there's something to solve here still but just want to confirm that

That is the problem i'm trying to address. I did not update/upgrade anything willingly. Any fresh install previously was working, now it's not.
Of course if i force the install of a previous version, it works. That is indeed a workaround, but i'm afraid many other developers in many packages would have to do it too.

i more meant an issue that when we remove packages, we should bump the major version possibly. though there may be reasons we don't do that im just not aware of.

i think i understand now.

you're getting problems because you've updated to the latest @types/keyv (since its under a patch version). that is a stub which doesn't really contain anything.

any code you then directly have which uses keyv will fail until you also upgrade keyv itself (so you pull in the new types).

similarly, any dependencies you have which depend on @types/keyv with a loose enough semver range will also pull in the stub. so they will no longer work unless they themselves upgrade the keyv they depend on too.

ultimately this drills down to the fact that it shipped in a patch version i think.

i don't think anything is really broken here. anyone who wants to upgrade should remove @types/keyv and upgrade keyv. i think its questionable though that this shipped in a patch version.

@43081j yes, i guess it boils down to:

• this landing in minor version instead of major. I didn't know who decided on semver in that case, so my bad.
  If you can add some input about it in the issue, as the change author - Removing typings for previously existing libraries breaks compilation of dependent packages #62793 , that would be great. My specific problem can be fixed by a workaround on my part, but i would like to prevent something like this from happening again.

• @types/cacheable-request having @types/keyv: "*" in the dependencies in the first place, which would mean this issue happens even in case this was released as a major version.
  I don't know if it's an another part of DefinitelyTyped principles. If so, it seems really misguided in nature, as there is really no way to guarantee forward compatibility for an infinitely long time, and semver is supposed to handle exactly that.
  Basically, IMO, any "*" in dependencies is a problem waiting to happen.

@Ansile - lets go ahead and revert this as it is breaking many older versions.

@jaredwray further explanation would be great.

the old version is available on NPM as expected, anyone who chooses to upgrade can use the shipped types.

what makes you believe this needs reverting?

any examples would be ideal, we really shouldn't blindly revert something which is logically sound. happy to help if there's something broken though

for what reason?

4.5 ships types:
https://unpkg.com/browse/[email protected]/src/index.d.ts

are the shipped types not correct?

just trying to understand as reverting this shouldn't be necessary except in one case: that the pre-shipped-types version had incomplete types here we now want to correct

which would mean the DT types were incomplete for <4.5. alternatively, your own types are wrong and you want to use the DT ones instead? either way, lets figure it out rather than blindly reverting it.

please lets track this is #62793 since we have two conversations going on at once atm