[rush] `rush version` bumps unchanged projects

mmkal commented 3 years ago

Summary

rush version bumps unchanged packages, when one of there devDependencies changes. This happens even with "useWorkspaces": true and the dependency is declared as workspace:*.

Repro steps

This repo has a script which reliably reproduces the bug: https://github.com/mmkal/rush-learning

git clone https://github.com/mmkal/rush-learning
rush install
./change-and-version.sh

The repo is set up such that:

there are three packages: pkg-a, pkg-b and pkg-c
pkg-b depends on pkg-a as a devDependency.
pkg-c has no dependencies of any kind, just to make sure there isn't some bump-everything situation going on
there are no version policies defined

What change-and-version.sh does:

ceates a branch
edits some code in pkg-a
creates a change file with rush change ...
merges the branch
prints the rush version command

If you run the printed command after that you should see the bug

Expected result:

Only pkg-a has its version bumped.

Actual result:

Both pkg-a and pkg-b have their versions bumped. pkg-b has a "Version update only" entry in its changelog

Details

Originally came from this zulip thread which was about rush publish. But it repros with rush version without the added complication of setting up a verdaccio registry. Turned into an issue on @octogonz 's suggestion.

Not sure, maybe when versioning, workspace:* is swapped for the "real" version?

Standard questions

Please answer these questions to help us investigate your issue more quickly:

Question	Answer
`@microsoft/rush` globally installed version?	5.35.2
`rushVersion` from rush.json?	5.35.2
`useWorkspaces` from rush.json?	true
Operating system?	Windows / (same behaviour seen on GitHub actions Linux)
Would you consider contributing a PR?	Maybe, if it's small
Node.js version (`node -v`)?	12.18.0

iclanton commented 3 years ago

This behavior was probably by design, but I'm thinking I agree with you and @octogonz that this may not make sense for devDependencies. However, I could see some projects wanting to be released if their devDependencies have changed. For example - a project that bundles another project, with the bundled project listed as a devDependency.

The code that marks dependent projects as "changed" is here. Just removing the "bump my version if one of my devDependencies changed" functionality will be slightly complicated because we still need to make sure the version update is reflected when something other than workspace:* is used.

@D4N14L has familiarity with this code.

octogonz commented 3 years ago

Yes, looking at this more closely, it is safest for rush version to always bump the downstream projects.

But in many (most?) cases it is clearly wasteful.

Just removing the "bump my version if one of my devDependencies changed" functionality will be slightly complicated because we still need to make sure the version update is reflected when something other than workspace:* is used.

The envisioned behavior would be to bump the package.json "dependencies" without bumping the "version" field. I feel like that logical is already built into rush version, so we should simply need to be careful about which steps get skipped.

octogonz commented 3 years ago

In order to improve this, we need to decide on the best design.

For example - a project that bundles another project, with the bundled project listed as a devDependency.

@iclanton you are right that whether to ignore devDependencies really depends on whether the particular change may influence the published output or not. Some more examples:

Upgrading the ESLint rules: does NOT influence anything, safe to ignore
Upgrading the shared TypeScript configuration: might influence the output, but usually not
Upgrading a library that is a dev-dependency because it gets bundled: always influences the output, never safe to ignore

So we could propose a new Rush setting like ignoreDevDependenciesWhenBumpingProjectVersions, but I don't think this is a good design. It's not a question of style of work in your monorepo. The switch really depends on individual dependency relationships.

Idea 1: Maybe we could introduce a project-specific setting that says "When you publish a new version of me, you do not need to publish bumps for other projects that list me as a dev-dependency." So you would configure true for a @example/my-lint-rules, but false for @example/art-assets-to-bunde.

Idea 2: Another idea would be for rush change to ask users about their change. Something like this:

$ rush change
Found changes for "pkg-a"
Describe changes, or ENTER if no changes: Updated the lint rules

The "pkg-a" project referenced in the "devDependencies" field for other projects.
When we publish a new version of "pkg-a", do we need to publish an update for 
those projects?  NO

mmkal commented 3 years ago

Might another option be to build if a dev dependency has changed, and store a hash of all the package files that aren't npmignored in changelog.json? Then if the hash is identical to last time, no version bump.

package.json would just need to have its devDependencies field excluded from the hash function input (if the files produced by build haven't changed, it's safe since devDependencies won't affect users).

octogonz commented 3 years ago

Might another option be to build if a dev dependency has changed, and store a hash of all the package files that aren't npmignored in changelog.json? Then if the hash is identical to last time, no version bump.

🤔 My first thought was "that's too complicated." But the more I think about it... it's a damn good idea.

mmkal commented 3 years ago

@octogonz I wonder if it would also solve #2263 and #2279 - assuming it's ok to make rush build a prerequisite for rush change, the package hash could be used to determine if there's any point creating a change file, more reliably and with less boilerplate than asking users to manually maintain a .changeignore file

mmkal commented 3 years ago

@octogonz @iclanton curious if this idea was discussed any further. Another thought occurred to me, which was that the publishableHash value (or whatever it might be called) could be useful in rush build, even: could we say there's no need to rebuild downstream dependencies if the package hash isn't affected by a rebuild? If I understand properly, that way changing a test or documentation file in a heavily-depended on package could save a huge amount of build time, without requiring any userland special configuration.

octogonz commented 3 years ago

Seems like the discussion has progressed enough that someone could create a PR to test out this idea.

We currently have 38 open PRs, so right now the maintainers should focus on getting them reviewed+merged. :-)

isc30 commented 2 years ago

hey, im running into this issue with a multi-version-policy project. Any progress?

isc30 commented 2 years ago

in my case, running rush version --bump -b master bumps every single package. It doesn't matter if its used as a devDependency or not, it bumps every package with a version policy

isc30 commented 2 years ago

found the cause: I never ran a proper bump+publish against master in that repo and it was detecting changes as it was the "first" one. After doing the initial bump+publish and pushing all the changes, the following ones are detected properly.

babusatti commented 2 years ago

Hi, @isc30 Can you help me with the rush version bump. I have configured versionPoliyname in version-policy.json and in rush.json files and while running rush version --bump, i can see the version is increasing but didn't see the commit in my repo.

ziofat commented 2 years ago

I don't think this situation only applies to devDependencies but also peerDependencies and even dependencies.

Consider the following scenario:

I have three packages, pkg-core, pkg-plugin and pkg-test. pkg-core has all basic functionalities and supports plugins. Current version is 1.4.0 pkg-plugin is one of the plugins, which have peerDependencies and devDependencies of pkg-core@^1.4.0. pkg-test is a test helper, which have dependencies of pkg-core@^1.4.0

No version polices.

When I add a new feature to pkg-core, its version bumps to 1.5.0. But as other packages, they do not require users to upgrade pkg-core. In fact, the lib files of pkg-plugin and pkg-test never changed. In this situation, pkg-plugin should not bump its devDependencies and peerDependencies because pkg-core@^1.4.0 is satisified with 1.5.0.

ziofat commented 1 year ago

I am still disturbing by this issue. My plugins are managed with the core package in a single rush workspace and they have a lot waste versions just because the peerDependencies and devDependencies updated.

isc30 commented 1 year ago

Hey @ziofat , are you using a nextBump in the version policies or not?

When no nextBump is speficied, all the projects that use the policy get updated together, think of it as a bunch of coupled projects like react, @types/react, react-dom, react-is, etc where you want all of them to have the same version together (even if nothing changed in one of them)

ziofat commented 1 year ago

@isc30 No, I'm not using bumpType. I am debugging through the PublishUtilities.ts file and it seems have workaround.

isc30 commented 1 year ago

That's the reason, you need to use a nextBump in the version policy and it will work as you want it. When no nextBump is present it will consider all the packages as a single "group"

ziofat commented 1 year ago

But I'm using rush version --bump based on changeset files. The pipeline will not know what nextBump should be.

ziofat commented 1 year ago

I think I've found 2 pieces of codes that related to this issue.

The first one is:

https://github.com/microsoft/rushstack/blob/8250ce6d1ab54cb3ef870a3935eac45e08aff6cf/libraries/rush-lib/src/logic/PublishUtilities.ts#L796-L804

This is doubtable when updating devDependencies. It only need to republish if the package was invoked in bundling.

The second one is:

https://github.com/microsoft/rushstack/blob/8250ce6d1ab54cb3ef870a3935eac45e08aff6cf/libraries/rush-lib/src/logic/PublishUtilities.ts#L446-L455

Do we really need to update the peerDependencies if there are no changes in current package? Usually we set a range of versions of peerDependencies like pkg-core@>=2.1.0<3.0.0 because we used the API provided since 2.1.0. Changing this version is unnecessary and may result narrower version range for users which may cause version conflicts.

This is actually my situation. I have a pkg-core which provides serveral APIs and update frequently. And I have a pkg-plugin which can be used with pkg-core to get more functionality but update occasionally. The new version of pkg-core would not affect pkg-plugin's ability. As a plugin, pkg-plugin defines pkg-core as peerDependency and devDependency.

We are not using git-flow so nextBump is not a solution. Our main branch have CI to continuously publish new packages.

Back to the code, I'm not sure how to resolve this based on the code I've found. Maybe a new field in version-policy.json to indicate if the project could affect building result if it is in devDependencies? And remove the line of code to update peerDependencies, it should be decided by developers.

isc30 commented 1 year ago

I agree that publishing all the downstream dependencies when the existing semver is still satisfied doesn't sound like the best solution for your use case, but when the version-policy doesn't have a nextBump it will unify all versions into a single one, that's why it republishes packages that only have a "dependency" type of change.

Example, in a simplified situation using direct dependencies:

pkg-core has a version 1.0.0
pkg-plugin has a version 1.0.0

package.json of pkg-plugin:

dependencies: {
  "pkg-core": "workspace:^*" // valid for anything 1.X.X
}

Now, when the pkg-plugin builds and publishes for the first time, workspace:^* will be changed to ^1.0.0 meaning, the package is compatible with any major version of pkg-core.

When pkg-core changes with a patch or minor, we could add an extra check when updating the downstream dependencies (pkh-plugin) and check if their current version already satisfies the type of change we are about to apply. If it already satisfies, we don't need to republish BUT that breaks the main functionality of "unified versions".

By definition, when no nextBump is specified in the version-policies.json, a unified version is used for that set of packages. If we make pkg-core publish 1.1.0 but leave pkg-plugin as 1.0.0 we have a problem where we have broken the unification of packages.

If you want to disable the unification of package versions, please add a dummy nextBump: patch to the version-policies.json and it will all start working as you expect.

Why did we introduce a unified version for a package group?

It's a very handy way of keeping lots of packages under control. Imagine you have a library that is split in hundreds of packages like react, react-is, react-dom, react-reconciliation, react-fiber, etc and you want people to use the same version for all react packages (lets say 18.0.0) to keep things simple. If we didnt have the "feature" that we also publish "dependency" changes, we would end up having a different version per package, which is incredibly difficult to maintain. So we decided to use the "no nextBump" as the indicator of "i want all the packages using this policy to be unified".

TLDR

If you want to disable the unification of package versions, please add a dummy nextBump: patch to the version-policies.json and it will all start working as you expect.

ziofat commented 1 year ago

I've tried that. Actually the only difference is it does not update downstream dependencies' version. But it still changed the peerDependencies to the latest one which is not what we want.

I'm aware how unified version works. react, nestjs, babel and even rush itself are using this version strategy. I believe that's why lockStepVersion comes for, does it? I'm using individualVersion so I think my packages should not apply to unified version.

In fact, pkg-core and pkg-plugin are not in same version policy. Maybe rush should only update downstream dependencies with in same version policy? It's unlikely for us to split plugins to another repo like what rollup did because we also have an app/website to integrate all packages.

ziofat commented 1 year ago

Also, nextBump actually defines what the version should be at next rush version --bump. If I set nextBump to patch then only the patch version is increasing, regardless what change type I've picked in rush change. It's not dummy.

isc30 commented 1 year ago

Ah sorry, I misunderstood your issue then, I thought you were using a lockStepVersion.

The behavior you are experiencing is the "safe" approach for individualVersion but I understand the concerns that if the already existing version selector satisfies the new version it shouldn't republish.

I can't speak for the owners of the repo but IMO it sounds good. We can try to optimize this case and prevent bumping patches if the updated dependencies still match the version selector, the change shouldn't be too big tbh.

Any thoughts @octogonz @iclanton ?

ziofat commented 1 year ago

The original issue is about a devDependency. The solution is to build and compare the output hash before bumping the version. However, it is too complicated for me to implement a PR for this.

My focus is mainly on peerDependency, which should not affect the build process and is more suitable to be manually defined instead of automatically maintained. I will submit a PR to disable the update of peerDependency in downstream dependencies if they are not in the same lockStepVersion version policy as the upstream. Is it sound OK for you? @octogonz

ziofat commented 4 months ago

Here again, another year. Any progress on this? I noticed that there is a 'help wanted' tag on this but I found the solution has not been throughly discussed yet.

microsoft / rushstack