Removing typings for previously existing libraries breaks compilation of dependent packages

[Ansile]

#62741 removed typings for @types/keyv library, replacing them with a readme file that suggests not using this package. This was released as a minor version, due to how automatics behind figuring out a new version work as of right now.

This causes errors like "Cannot find type definition type for keyv" in compilation for packages that are dependent on @types/keyv -

That is to the issue in typescript - microsoft/TypeScript#27956, which basically requires a @types package having index.d.ts to not break compilation.

Removing this library altogether would be good, but not possible due to it being the transitive dependency for my package.

IMO, this change breaks semver and such updates should be released as major version in the future.

The other part of the problem is @types/cacheable-request package, that had (before it was also removed from this repo) "@types/keyv": "*" in dependencies.

It means that:

• publishing this as a major version wouldn't have helped in this particular case (as * matches any major version)
• a similar (just lesser) breakage could have happened if some other details of @types/keyv implementation changed and a major version was released. No package can realistically guarantee backwards compatibility for all future versions up to infinity. That's what semver is for.

Proposed short-term fix:

• republish @types/cacheable-request as a patch version, with correct dependencies - "@types/keyv": "^3.0.0" and some specific major versions in place of other wildcards.

Proposed long-term fix - do either one or both of these:

• stop the removal of types in npm (just remove them from git and deprecate the latest version when it happens)
• (preferred) prohibit the usage of * in dependencies of @types packages, and always publish the "removed" package as a major version

Similiar issues (caused by the same root problem):

• [@types/responselike] Cannot find type definition file for 'responselike' #62111

[Ansile]

If anyone looks for a workaround: pinning a specific version: "@types/keyv": "3.1.4" (in dependencies/devDependencies if you're using npm<8, or in overrides if you're using npm8) should be enough.

[faern]

This indeed causes issues. For example electron/rebuild#1037 and electron-userland/electron-builder#7211 (the same thing reported to two packages)

Combining wildcard version dependenices (depending on keyv *) together with "removing" a package (publishing an empty package?) is by definition going to cause these issues. Removing parts of the API (all of it) is definitely semver breaking, and should not be done in a minor/patch release (it should not be done at all IMO). But even if it was being released as a major, since @types/cacheable-request depends on keyv * it would still break.

The way I see it this repository did two things wrong that caused this:

• Never specify * as the version of your dependencies in a published packages.
• Never remove a published package. It's really weird that npm allows this. The community should have learned something from left-pad?

Proposed fix

Currently there are "fixes" proposed, where programs that only has keyv as a transitive dependency have to pin its version. These are pretty ugly workarounds. I instead suggest that definitely typed do these two things:

• re-publish @types/keyv with the exact same API/code it had before it was removed, as a patch upgrade again.
• Publish a new version of @types/cacheable-request that specifies a proper version of @types/keyv with proper semver caret notation.

Extra note: No need to keep the code around in this repo just because it's left published on npm. That's what git is for.

[Ansile]

@faern 100% agree. I only proposed a workaround earlier, but this certainly needs a proper fix from DefinitelyTyped team by producing some changes to publishing infrastructure (for example, disallowing * for another @typesdependency version).

I initially missed the "@types/keyv": "*" part, thinking that the issue was with semver in the latest publish, because it was a minor version (and forgot to update the issue because of an abruptly ended conversation on the merge request that introduced this change).

But because of * in dependencies for @types/cacheable-request this was basically a ticking bomb - similar issue could have happened without removing typings for keyv, simply because of the fact that no package can realistically guarantee backwards compatibility for all future versions up to infinity. That's what semver is for.

I'll update the issue comment to include all of these points.

[43081j]

sorry just catching up on this.

i do believe all the above, but im confused since cacheable-request didn't have a package.json.

you can see that in #62102.

even way back to the original #32682, there's no dependencies specified.

im gonna guess that DT does some magic in these cases where there are implicit dependencies (i.e. the fact it imports from keyv but doesn't declare it explicitly), and that magic will add * as a semver specifier since it will be generated.

solution

reverting the keyv removal is not the right thing to do here.

if anything, we should:

• re-introduce cacheable-request's types
• add a package.json to it
• publish new version (minor or, even better, patch)
• remove cacheable-request's types

have opened #63097

[Ansile]

@faern i added all of the points into the original issue.
I liked the @43081j idea for the immediate fix a bit better though (and he already introduced a PR, which is great), so i included it in the fixing proposals.

It seems from the number of linked issues that this is a pretty widespread breakage. It's also really not limited to one package. I linked a similar issue to this one - #62111 (there might be more that's harder to find? I'll investigate later). And @43081j's fix, while helping with the current problem, will do nothing to fix the underlying issue.

Maybe someone knows who of the DT maintainers can help with mitigating the root problem and avoiding the breakages in the future? I'm not really familiar with local infrastructure, so i wouldn't know who to tag.

Also, i removed a reference to the specific package (as i've now confirmed that there have been other breakages), and i mostly care about this being fixed for all of us (and not happening again), not fixing this particular problem for me (else i would've put this into discussions).

[43081j]

every linked issue is caused by the same thing, but you are right they're not entirely cacheable-request (one is about responselike).

responselike was originally removed but then reverted because this same problem occurred.

a package depended on it implicitly, so had * specified and pulled the stub package once it was removed. arguably, we shouldn't have reverted responselike's removal but should've fixed up the dependent package like we are here.

IMO it would be better to open a new, more focused issue btw. this one has gone off on a bit of a tangent about cacheable-request. (edit: you solved this i think by renaming the issue)

the overall issue is one of process/reviewing i think: if we remove a package from DT, and there are any dependents which are implict (so they specify * as a semver range), they will break once we publish the removal.

i feel like there's not much we can do about that, though. maybe one of:

• update the dt tools to use the latest version when specifying implicit dependencies rather than *
• disallow implicit dependencies going forward (a package.json must be given if there are any)

[faern]

reverting the keyv removal is not the right thing to do here.

What's the rationale behind publishing stub packages in the first place? They are by definition semver breaking 😢 and can cause issues like this. I don't see any upside of it.

and that magic will add * as a semver specifier since it will be generated.

This seems like a pretty crazy system as it effectively says any version at all will work. Effectively locking the dependency from ever doing a major release with a breaking API.

Both practices lead to breakages down the line. The publishing of stub packages are just a faster way to get there.

the overall issue is one of process/reviewing i think: if we remove a package from DT, and there are any dependents which are implict (so they specify * as a semver range), they will break once we publish the removal.

It's definitely a process issue. But not a review issue. If you stopped publishing "removals" of packages this would not happen.

• update the dt tools to use the latest version when specifying implicit dependencies rather than *
• disallow implicit dependencies going forward (a package.json must be given if there are any)

This is good, if you do it with caret notation. Say the latest version of the dependency is 1.2.3 when you start using it. Then specify the dependency as ^1.2.3. This allows npm to pull in all semver compatible versions. This allows the dependency to in the future publish a 2.0.0 with a new API without the depender breaking like this. Please don't solve this with version pinning (=1.2.3) as that does not allow the package to pull in later updates to the dependency. Version pinning also makes the dependency tree blow up. Since if someone else does the same a bit later, they might pin =1.2.4 and both versions must be downloaded and included, where one should be enough since they are compatible with each other.

I'm new to npm, but coming from crates.io this seems extremely dangerous. There you are 1) not allowed to publish packages with * dependencies, nor delete already published packages.

[43081j]

What's the rationale behind publishing stub packages in the first place? They are by definition semver breaking 😢 and can cause issues like this. I don't see any upside of it.

The initial issue suspected the stub package was a patch version, it wasn't. It gets published as the version of the source package which first ships types (which you'd hope is at least a minor bump if the source maintainers are following semver, usually its actually major).

You could argue this should always be a major bump rather than hoping the source package gets it right.

DT publishes a stub package so we're able to mark it as deprecated i think.

This seems like a pretty crazy system as it effectively says any version at all will work. Effectively locking the dependency from ever doing a major release with a breaking API.

yes i agree, the one thing to come out of this IMO should be that dependencies must be explicit. Or if thats too drastic, infer the latest version and use that as a range (with ^).

[43081j]

i did a quick script to check for type definitions with implicit dependencies, there's a lot... so not too sure what the solution is there.

if we had a rule of "all dependencies must be explicit", we'd have a huge task ahead it seems.

e.g. take a random one, ag-auth depends on jsonwebtoken but doesn't even have a package.json. by the looks of it there's more type defs like that than ones with explicit dependencies...

[jaredwray]

@43081j - my proposal was to bring back the current @types/keyv then we will add in version 4.2.x and 4.5.x to keep things from breaking.

We will then take the project and move to a breaking change as of 5.x.x coming out end of year. Thoughts?

[43081j]

The keyv types were never the problem (or removal of them).

cacheable-request was the culprit. It was removed some time ago and had an implicit wildcard dependency.

I'm reintroducing that in a pr here so I can add an explicit dependency, then I'll remove it again.

At that point everything for that particular case will be fine and resolved. However this is a wider issue now so still needs discussion.

@jaredwray if there's still some concerns would you mind discussing with me in #63097 so we can keep this one about the general problem?

[faern]

DT publishes a stub package so we're able to mark it as deprecated i think.

But why remove the code? It can be a patch release with just the metadata changed to deprecated.

[43081j]

not something i know. i can only guess its because the DT maintainers want to publish a readme to explain why it has been deprecated at least.

@sandersn or @andrewbranch will know

in case either of you take a look at this, think the summary is:

• many packages have implicit dependencies we populate with a semver range of *. if we ever remove the thing they depend on (assuming it is a DT package), they will automatically depend on the newly published stub package
  • maybe resolve the latest version instead and depend on that for implicit dependencies? (lot of overhead i imagine though having to resolve that via npm...)
• a removed DT package is bumped upto the version of the source package that ships types (it seems), this can sometimes be something silly like a patch version if the source maintainers don't follow semver well
  • maybe we just force a major version if the supplied "notNeeded" version didn't?

edit:

@faern i kinda get it. the stub DT package actually specifies the source package as a dependency. with the intention of pulling that in if you install the types package i imagine.

[sandersn]

I tried reading through the issue and there is too much for me to understand here. Can you help me get started by answering a few questions:

• Was the only broken dependency between @types/cacheable-request and @types/keyv?
• Were there other packages that depended on @types/keyv@* and broke?

The reason I ask is that @types/cacheable-request itself was stubbed out and deprecated a month before @types/keyv. I would expect that updating keyv and @types/keyv would also updated cacheable-request and @types/cacheable-request.

The reason that the stubs work the way that they do is that tsc ignores @types/X whenever X has types. So if keyv@4 has types, @types/keyv@4 doesn't do anything: the @types/keyv stub depends on keyv to make sure of this. Which leads to the next question:

• How did @types/keyv get upgraded to 4 without also installing keyv@4, which has types?
• If it did, then why did tsc not see keyv and instead look at @types/keyv for types?

[43081j]

Only cacheable request was broken.

Nothing else really broke since everything else had a sensible dependency semver range.

I think the tldr of what happened is this:

• Cacheable request was added to DT, it did not specify a dependency on keyv even though it had one
• The resulting npm package had a keyv types dependency automatically added with * as the version constraint
• Cacheable request was removed from DT
• keyv was removed from DT

This meant that people still using the old (since removed) cacheable request types would pull down the new stub package of keyv's types in some situations (not sure exactly which). Because the version constraint on it was * remember.

Their old code would be using the old keyv which doesn't ship types. So it would break because of this

Also sorry the issue has been difficult to follow. Didn't help that two issues were going on at once until recently in here.

[RyanThomas73]

I think the tldr of what happened is this:

Yep, matches the sequence I found when investigating this error with the @types/pino packages.

1. @types/pino (which has been removed+stubbed since v7) had implicit dependencies on @types/pino-pretty and @types/pino-std-serializers and was published to npm with those dependencies listed as wildcard * version.
   Example: https://www.npmjs.com/package/@types/pino/v/6.3.12

   {
       "name": "@types/pino",
       "version": "6.3.12",
       // .. other stuff removed for brevity ...
       "dependencies": {
           "@types/node": "*",
           "@types/pino-pretty": "*",
           "@types/pino-std-serializers": "*",
           "sonic-boom": "^2.1.0"
       },
   

2. The @types/pino-pretty and @types/pino-std-serializers we're recently deleted+stubbed.
   Now consumers of @types/pino@6.x versions are getting these stubs transitively due to the version wildcard and thus getting the TS2688 error.
   chore (pino-std-serializers): remove redundant types #62975
   chore (pino-pretty): remove redundant types #63169

3. Additional packages that reference and re-export @types/pino@6.x will also pull through the stubs transitively and have this problem.
   e.g.
   Dependency Bug - @types/pino breaking typescript in consumers pact-foundation/pact-js-core#417

[43081j]

😭 I too am responsible for removing those but couldn't have really known about the wildcard dependencies since they weren't in DT.

We can fix it @RyanThomas73 just the same, by republishing pino's types with explicit dependencies then removing it.

But yeah this has happened twice now so would be good to come up with a solution to avoid it in future.

Requiring explicit dependencies makes sense to me but would be a huge change