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

Rush Stack dependencies have locked version ranges; should they be lockstep or loose? #2599

Open
octogonz opened this issue Apr 8, 2021 · 2 comments
Labels
general discussion Not a bug or enhancement, just a discussion

Comments

@octogonz
Copy link
Collaborator

octogonz commented Apr 8, 2021

Problem

When upgrading @rushstack dependencies in our monorepo, we frequently get errors like:

(TS2322) Type 'import("C:/Git/my-repo/common/temp/node_modules/.pnpm/@rushstack/[email protected]/node_modules/@rushstack/node-core-library/dist/node-core-library").Terminal' is not assignable to type 'import("C:/Git/my-repocommon/temp/node_modules/.pnpm/@rushstack/[email protected]/node_modules/@rushstack/node-core-library/dist/node-core-library").Terminal'.

This case happened because @rushstack/rush-lib and @rushstack/ts-command-line both depend on @rushstack/node-core-library without a loose SemVer range (e.g. ^3.36.0).

👉 Essentially, if you upgrade one Rush Stack package to the latest, you must upgrade all other Rush Stack packages across your monorepo.

And to do that, I need to consult npmjs.com to determine the latest version for each package. Downgrading would be rather complex.

Possible improvements

  1. Treat the Rush Stack packages as an SDK. We could enable lockstep versioning, so (a relevant subset of) all the packages get published with the same version number. I would still need to keep them all consistent, but @rushstack/ts-command-line, @rushstack/node-core-library, etc. would all get set to the same number 3.36.1 rather than a vector of arbitrary numbers.
    - OR -
  2. Loosen up the version ranges. For example, @rushstack/ts-command-line would depend on @rushstack/node-core-library version ^3.36.1 instead of 3.36.1

Remembering back, there were several reasons why the versions originally got locked:

  • Nondeterministic upgrades: A fix was shipped for SPFx. Customers changed their @microsoft/sp-build-core-tasks dependency to be the new version, but when they ran npm install it would not upgrade indirect dependencies unless they deleted their package-lock.json file. Eliminating the version range ensured that the fix would get installed.
  • Nondeterministic installs: For similar reasons, simply running npm install --global @microsoft/rush on Monday might install @rushstack/node-core-library 3.36.0 on Monday, but on Tuesday it would install 3.36.0, even though the version of Rush had not changed. The person who installed the tool on Monday might report a bug that cannot be reproduced by the person who installed it on Tuesday.
  • Internal APIs: When breaking changes are made to an API that is marked as @internal, these are generally considered to be a PATCH version bump. This could lead to actual errors when the SemVer range was too loose, particularly since dependencies can be bidirectional (e.g. breaks can occur regardless of whether the caller is newer or older than the callee).

So if we go with #2 we should address these cases. Maybe now there are other options for handling them (e.g. bundling Rush's dependencies).

CC @iclanton @D4N14L @dmichon-msft

@octogonz octogonz added the general discussion Not a bug or enhancement, just a discussion label Apr 8, 2021
@iclanton
Copy link
Member

I'm good with #2, and I think bundling Rush's dependencies is a good idea.

@dmichon-msft
Copy link
Contributor

Having consumed dependencies from monorepos that produce with lockstep and those that produce with loose versions, I find lockstep much easier to work with, and loose to be a nearly unending source of frustration (these tend to result in package version duplication and collisions), so I'd personally prefer lockstep.

If we go with loose versions, the best thing to do would be to also publish a JSON manifest of "This repo published these packages with these versions and these declared dependencies" so that tools that pick up new versions can upgrade all published packages to a consistent matched set. It's useful even if we don't make the change, to be honest.

@iclanton iclanton moved this to General Discussions in Bug Triage Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general discussion Not a bug or enhancement, just a discussion
Projects
Status: General Discussions
Development

No branches or pull requests

3 participants