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

Allow peer dependencies with stricter version ranges #433

Open
eps1lon opened this issue Apr 12, 2022 · 28 comments
Open

Allow peer dependencies with stricter version ranges #433

eps1lon opened this issue Apr 12, 2022 · 28 comments

Comments

@eps1lon
Copy link
Contributor

eps1lon commented Apr 12, 2022

Continuation of microsoft/types-publisher#431.

Current state

When you import from a package hosted in the DefinitelyTyped repository in a package that's also hosted in DefinitelyTyped, the types publisher will automatically declare a direct dependency on that types package matching any version. For example, if you have a package react-library that imports from react then the package.json of @types/react-library will look something like this:

{
  "name": "@types/react-library",
  "dependencies": {
    "@types/react": "*"
  }
}

Problems

Combination of the described problems are encountered in

duplication of @types/* packages

Given an existing module graph similar to

[email protected] /home/eps1lon/Development/throwaway/types-peer-deps
├─┬ @types/[email protected]
│ └── @types/[email protected] deduped
└── @types/[email protected]

If you want to upgrade to React 18 you would need to update @types/react with npm install @types/react@18.
However, npm (and yarn and probably other package managers) make sure that this install does not alter the dependency tree too much (if you're using lockfiles). What you end up with is something like

[email protected] /home/eps1lon/Development/throwaway/types-peer-deps
├─┬ @types/[email protected]
│ └── @types/[email protected] 
└── @types/[email protected]

Now your dependency tree contains multiple version of a types package. In this specific case these packages are incompatible.
To fix, one would have to apply one of the workarounds described in facebook/react#24304 (comment) which isn't trivial at all.
After all, package managers are supposed to resolve these issues. But they can't if we provide them with inaccurate information.

* is not forwards compatible

While currently published packages might work perfectly fine with all published versions of @types/react at the time, there's no guarantee that continues to work. It essentially requires that no breaking changes are made at all which defeats the purpose of SemVer major releases.

Proposal

Allow peer dependencies

React is generally a peer dependency and therefore it types should as well.
If we could move @types/react from dependencies to peer dependencies with

"peerDependencies": { 
  "@types/react": "*"
},

we would ensure types deduplication since the package owner would defines the version of @types/react a single time (via dependencies or devDependency.

Allow constraining @types/* dependencies to something stricter than *

Either let maintainers decide the range manually or infer the range from the versions it's currently tested on. For example, @types/react-library runs tests with @types/react v15, v16, v17 and latest. In that case we check what version the current release has (or will be) and declare that.
This way an update to @types/react-library will also bump versions of dependencies and therefore propagate bug fixes and features of those dependencies upon release.

related issues

@andrewbranch
Copy link
Member

Can you clarify whether you think the two parts of your proposal ("Allow optional peer dependencies" and "Allow constraining @types/* dependencies to something stricter than *") are both necessary or if only one or the other is necessary? I could be wrong, but I thought the latter can be done today by adding a package.json and specifying the version range. (This currently requires adding to allowedPackageJsonDependencies.txt, but I think we’re open to automatically allowing all @types packages to be listed.)

@eps1lon
Copy link
Contributor Author

eps1lon commented Apr 12, 2022

Optional peer dependencies are the more pressing issue. If they keep the "*" range then the package owner will still end up with a single version and they explicitly declare that.

Getting rid of the "*" range is still important for any package that's no a peer dependency.

So in my mind these things can be implemented independently. But optional peer dependencies is "more necessary" than strict version ranges if that makes sense. I just grouped them in a single issue since the concrete examples are affected by both.

I could be wrong, but I thought the latter can be done today by adding a package.json and specifying the version range.

I was under the impression that this didn't work. But it's been a few years since I tried and maybe it's now possible.

@Methuselah96
Copy link
Contributor

Methuselah96 commented Apr 13, 2022

I think it would be helpful to narrow this issue to just be about allowing optional peer dependencies and move the stricter version ranges to a separate issue. Getting rid of the "*" will certainly be helpful, but it pales in comparison to the urgency and importance of allowing optional peer dependencies and seems to really be addressing a different problem that is much less urgent to get fixed in my opinion.

@Methuselah96
Copy link
Contributor

Methuselah96 commented Apr 13, 2022

Note that allowing optional peer dependencies would also allow us to revert DefinitelyTyped/DefinitelyTyped#49914 which would be great since that solution is not ideal. Actually not since the types would not type-check properly if @types/react-native was not present.

@weswigham
Copy link
Member

weswigham commented Apr 13, 2022

Some thinking here is clearly off - if your types depend on react's types, the dependency is in no way optional, it's mandatory. An "optional" peer dep would only serve to break consumers of the package's types, since they'd suddenly be missing required type information. I can understand wanting to elevate from a dependency to a peer dependency, since people are used to react itself being a peer dep, but all that actually does for types is elevate a (potential! not guaranteed) compile time type conflict to an install time peer dep version conflict - the underlying conflict still exists.

Moreover, all of DT is tested to be internally consistent, so the latest versions of all packages on DT should always work together - enumerating how far back in time, and for what version ranges, that holds is practically impossible over a shifting set of dependencies, hence the * version ranges to allow any version to be installed (thus, deferring to package manager power users in case of esoteric mix-old-with-new dependency relationships). On top of that, using * versions allows us to avoid republishing all dependents of a package when it updates just to update the version strings - which we would always need to do, since, again, DT is kept internally consistent at the latest version of every package.

What's odd, to me, is that while a package manage is willing to update the outermost package in its tree, why it would be unwilling to update the inner one even though the version specifier allows it? Especially if it already deduped the versions to a single copy? (The package manager seems like it would have to do more work to move the old install to a new location!) Minimally, I know if you blow away your lockfile and install again, they'll unify to the single valid top-level install again, which is decidedly not what would be the case of version ranges were stricter - you'd just be guaranteed to have the version conflict.

@weswigham
Copy link
Member

weswigham commented Apr 13, 2022

TL;DR: We'll never allow optional peer dependencies - those don't make any sense for types. Required (and autoinstalled) peer dependencies make more sense, but don't seem to carry more meaning than normal deps do for our usecases - the peer relationship at install time is not strictly required (all it does is elevate when conflicts occur to install time). Stricter version ranges (not *) would seem to be counterproductive to solving the issue you state, since they'd just lock-in version conflicts rather than allow them to be resolved. Thus, taking that in all up, I'd just question why a package manager refuses to use the minimal allowed install set of packages when you update a single top-level package versus when you reinstall the whole dependency tree. Optimizing for "minimal lockfile change" seems like a weird, quirky difference between an install/add and a reinstall.

@Methuselah96
Copy link
Contributor

Methuselah96 commented Apr 13, 2022

The root issue is that package managers will try to install the latest version allowed within a range. So if you have an app that uses @types/react@17 and then install or upgrade a library or types package that depends on @types/react@*, it will install both @types/react@17 and @types/react@18. A peer dependency "solves" this issue by using whatever version of @types/react the app already has installed.

I agree making it an optional peer dependency is probably a red herring and probably got carried over the by the discussion of how non-@types packages should depend on @types/react.

@Methuselah96
Copy link
Contributor

Basically, it's easier to change it to a peerDependency then reconsider how all package managers currently work.

@weswigham
Copy link
Member

A peer dependency doesn't "solve" the problem though - if you replace * with 18, 17 and 18 will still conflict, just at install time instead of compile time. It's just more publishing work for us.

@Methuselah96
Copy link
Contributor

I'm just talking about the peer dependency part of this proposal, I think the discussion regarding * vs 18 should be separate.

@Methuselah96
Copy link
Contributor

I think it should be a peer dependency on *.

@eps1lon
Copy link
Contributor Author

eps1lon commented Apr 13, 2022

Some thinking here is clearly off - if your types depend on react's types, the dependency is in no way optional, it's mandatory.

Yeah for @types/* packages it's not optional. This approach was taken over from runtime libraries where types are optional. But required peer dependencies still make sense simply by virtue of them existing. If the types want to reflect the runtime then they need to reflect runtime dependencies. Setting dependencies with the * range does not reflect runtime dependencies.

Moreover, all of DT is tested to be internally consistent, so the latest versions of all packages on DT should always work together

At the time of the publishing,yes. But not for eternity which * is impliying.

What's odd, to me, is that while a package manage is willing to update the outermost package in its tree, why it would be unwilling to update the inner one even though the version specifier allows it?

Because that would change the code of the an outermost dependency even though it wasn't updated. If A depends on B, my app depends on A and B and I want to update B then the B that A uses should not change if I have a lockfile. That's one reason why lockfiles exist.

But that's not an arugment we have to have. That's simply the reality of virtually every JS package managers. It's not constructive to ignore that reality.

Stricter version ranges (not *) would seem to be counterproductive to solving the issue you state, since they'd just lock-in version conflicts rather than allow them to be resolved.

That's inaccurate. I'm not asking for exact version ranges. I'm asking for stricter version ranges than * e.g. ^17.0.0 which can still be resolved.

I'm not sure how I can better describe the problem. There's a current, concrete example why "dependencies": { "@types/react": "*" } is incorrect. If you already disagree that there is a problem then we don't need to discuss potential solutions.

A peer dependency doesn't "solve" the problem though - if you replace * with 18, 17 and 18 will still conflict, just at install time instead of compile time.

It solves the problem by allowing packages to resolve the conflict. They need to publish updates if they support later versions. The problem that's solved is at the app level where we currently have no workaround aside from lockfile surgeries.

@eps1lon
Copy link
Contributor Author

eps1lon commented Apr 13, 2022

I think it should be a peer dependency on *.

Every package depending on @types/react should have had a peer dependency on v17 since they have no idea if they would be compatible with v18. After the v18 release these dependends could've been updated to support v18.

Otherwise you can create dependency graphs on install (e.g. fresh install of a boilerplate without lockfile) where incompatible types are installed because someone put @types/react in there when they were compatible with v17 being latest but incompatible with the later v18 release.

@weswigham
Copy link
Member

weswigham commented Apr 13, 2022

Semver is a lie for types - semver compatible runtime changes in the underlying package can create breaking types. Semver incompatible versions of the underlying package (due to behavioral differences) can easily have the same or compatible types. Major.minor on DT literally only exits as a reference on the package to map back to the major.minor of the real package the types are for, so it's easy to know what types you probably want to fetch.

Hence why the only stable state for types is latest everything, because that's the only thing we've tested to work.

@eps1lon
Copy link
Contributor Author

eps1lon commented Apr 13, 2022

Major.minor on DT literally only exits as a reference on the package to map back to the major.minor of the real package the types are for, so it's easy to know what types you probably want to fetch.

If I add @types/react-formik today in a React v17 codebase I will get @types/[email protected]. If I do that 5 years from now it'll be @types/react@25 which may have a completely different API. That is incorrect. I proposed a solution that has been working for JS runtime libraries ok-ish for years. Just because it sometimes breaks down, does not justify declaring that part as incorrect. You can't bring types to the JS ecosystem if you ignore a fundamental part of that.

If you say that SemVer is a lie for types, why do DT packages sometimes declare stricter versions than * e.g. @types/react depends on csstype@^3.0.0. If that's incorrect, then we should disallow it to avoid future confusion.

@weswigham Do your statements reflect the opinion of the TypeScript team?

@weswigham
Copy link
Member

Because csstype is hosted off DT, so updates independently off it and actually follows semver (for its types) so is stable to depend on a major version of. Menawhile on DT, a patch version is very often "semver breaking" to most people. DT's versioning is really just monotonically increasing numbers in the patch version slot, where only the latest versions at time of publication are guaranteed to work together because that's all that was tested.

@Methuselah96
Copy link
Contributor

Methuselah96 commented Apr 13, 2022

It still feels like this should be two separate issues so we can narrow our discussion. The problem/solution of dependencies vs. peerDependencies is distinct from the problem/solution of how strict the version ranges should be. They just both happened to be problems that arose from the breaking changes in @types/react@18. Having more strict versioning will not solve the problem of having duplicate versions of @types/react installed by a package manager and using peerDependencies will not solve the problem of * not being forwards compatible.

@weswigham
Copy link
Member

Absolutely, the peer dep thing and the not-* thing a definitely two different discussions. I'm at least sympathetic to required autoinstalled peer deps, even if I don't see how they'd actually solve the underlying bad package manager behavior. (DT very much relies on dependency hoisting and unification.) While the * thing is essentially a DT FAQ entry I'm regurgitating.

@weswigham
Copy link
Member

On the peer dep thing, we have an age old issue on the (archived old version of the) types-publisher repo that links to most other discussions of the topic. Past guidance has been to use yarn resolutions (or just updating your lockfiles) to fix any problems which arise. Nowadays maybe we can maybe revist - it likely depends on how much we wanna support users still running aribtrary old versions of npm - a lot of the issues with peerDependencies now is simply that it's meaning has changed so much over time that it's not safe to use in packages that may actually get used in old projects or environments.

@fbartho
Copy link

fbartho commented Apr 13, 2022

DefinitelyTyped is a clever service for the TypeScript Ecosystem. The real "correct" fix is for types to be shipped by the official runtime-packages instead of being provided by DefinitelyTyped. Then they would be versioned exactly like the packages they belong to.

In practice, many DefinitelyTyped definitions are maintained by external communities, and aren't necessarily shipped simultaneously with their source-packages, there are always going to be warts and weak points. Starting with *-versions was a good compromise initially, because many types packages really don't care what version, or rarely have breaking changes. Unfortunately, that heuristic has failed today with a popular package that had breaking changes, and so *-package deps didn't work.

There's no point throwing the baby out with the bathwater. Let's refocus this ticket on the discussion of if "peerDependencies with stricter version ranges" is a valid enhancement to DefinitelyTyped capabilities that is worth the cost, and if there are obvious traps that we will fall into.

An additional point to discuss is the impact if "all packages" follow this recommendation. Do we lead to excess thrash on DT's repo? (Because people have to make new versions to increase the upper bound every time the source package updates?) Other problems?

@weswigham
Copy link
Member

I mean, we technically make exceptions to the * versioning thing on DT all the time - an amazing number of dt packages are on the dt allowlist of external deps - expressly to allow types to continue to be maintained for old versions of packages. There's a huge caveat that we tell anyone doing it though, and it's that when you do that, you lose the ability to update your dependents and dependencies in lockstep, and can deadlock your ability to fix issues. Instead, iirc, we have the major-version-folder thing which can be paired with major-version-folder paths mapping redirects in the tsconfig to explicitly make maintained versions of types for old packages depend on types for old versions of other packages. The external dep list is essentially for an unmaintained dependency (eg, because the types are no longer maintained on DT and so don't exist to be updated anymore - the last published version is the last there'll ever be), which is always a dicey choice.

@eps1lon eps1lon changed the title Allow (optional) peer dependencies with stricter version ranges Allow peer dependencies with stricter version ranges Apr 28, 2022
@Methuselah96
Copy link
Contributor

Looks like Yarn is experimenting with trying to install the minimal number of versions that satisfies all ranges (source) which could help this situation.

@paulius-valiunas
Copy link

Looks like Yarn is experimenting with trying to install the minimal number of versions that satisfies all ranges (source) which could help this situation.

that won't help if you have dependencies specifically on multiple versions of react in your monorepo.

@Methuselah96
Copy link
Contributor

The more common case is where only a single version of React is being used, but multiple versions of @types/react are unintentionally being installed. Having multiple versions of @types/react installed will produce type errors, even if the issues here are resolved, which seems like a separate (and more complex) issue than what's being discussed here.

@dgoldstein0
Copy link

Hey folks, bumping this thread because it seems to have stalled.

I would also like to raise a variant of the problem that I'm having in my company's monorepo: we're trying to support having different (major) versions of react for internal applications, but sharing our testing framework and build tools - so we all use the same webpack, typescript, jest, storybook etc. So we're trying to use yarn workspaces for this. And this brings me to:

Past guidance has been to use yarn resolutions (or just updating your lockfiles) to fix any problems which arise.

Resolutions has been a usable workaround for non-workspace usage of yarn - every package.json got it's own lockfile and therefore could specify it's own resolutions for @types/react to force deduplication. However, we're trying to adopt workspaces so that we can use yarn constraints and more easily share our build and test tooling, and this has put us in a bind - all of our packages in the same workspace is clearly optimal for the type of sharing we want to do for build & dev tools, but leaves us doing very ad-hoc resolution overrides of @types/react as all our different internal applications are on various react 16, 17 and 18 variants, and yarn resolutions allow us only to override resolution of specific semver specifiers globally - not on a package by package basis within a workspace. So a dependency on "@types/react": "*" can only mean one particular version of @types/react anywhere in the workspace.

What's the main objection to allowing @types/react as a peerDependency? testability of the types?

@jakebailey
Copy link
Member

jakebailey commented Jun 25, 2024

After the big pnpm monorepo restructuring, I don't believe there's a technical blocker for DT packages to list peer deps; every package has a package.json and sees every other DT package via those deps.

At this point, I personally think it would be safe to allow them on packages where they're clearly intended to be installed with another package, e.g. plugin packages or react (where the user is expected to install react anyway). I had meant to comment here after that refactor (this has been brought up on the DT channel on the TS discord previously), but hadn't had time.

(I am saying this conditionally, since I think it would be a bad idea to use peer deps for everything given there are still package managers which do not auto install them.)

@dgoldstein0
Copy link

sweet. so what's the road forward here? I would like see the ecosystem get to a place where all the packages that specify react as a peer dep have their corresponding @types/ package use @types/react as a peer dep. I imagine there will similarly be use for webpack, rollup, etc. ecosystems to do the same

@jakebailey
Copy link
Member

Realistically, just a code change to this repo to process peer deps, put them through the same checks as other deps, etc, which will be similar to code which checks devDeps (also new).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants