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] rush version bumps unchanged projects #2385

Open
mmkal opened this issue Dec 8, 2020 · 25 comments
Open

[rush] rush version bumps unchanged projects #2385

mmkal opened this issue Dec 8, 2020 · 25 comments
Labels
enhancement The issue is asking for a new feature or design change help wanted If you're looking to contribute, this issue is a good place to start! repro confirmed The issue comments included repro instructions, and the maintainers reproduced the problem rush-publish-requirements This issue describes user requirements that we should consider in the new design for "rush publish"

Comments

@mmkal
Copy link
Contributor

mmkal commented Dec 8, 2020

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

  1. git clone https://github.com/mmkal/rush-learning
  2. rush install
  3. ./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
Copy link
Member

iclanton commented Dec 9, 2020

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 octogonz added enhancement The issue is asking for a new feature or design change repro confirmed The issue comments included repro instructions, and the maintainers reproduced the problem labels Dec 10, 2020
@octogonz
Copy link
Collaborator

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
Copy link
Collaborator

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

@octogonz octogonz added the needs design The next step is for someone to propose the details of an approach for solving the problem label Dec 10, 2020
@mmkal
Copy link
Contributor Author

mmkal commented Dec 10, 2020

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
Copy link
Collaborator

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
Copy link
Contributor Author

mmkal commented Dec 14, 2020

@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
Copy link
Contributor Author

mmkal commented Jan 28, 2021

@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 octogonz added help wanted If you're looking to contribute, this issue is a good place to start! and removed needs design The next step is for someone to propose the details of an approach for solving the problem labels Feb 18, 2021
@octogonz
Copy link
Collaborator

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
Copy link
Contributor

isc30 commented Nov 25, 2021

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

@isc30
Copy link
Contributor

isc30 commented Nov 30, 2021

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
Copy link
Contributor

isc30 commented Jan 7, 2022

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
Copy link

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
Copy link
Contributor

ziofat commented Jun 30, 2022

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
Copy link
Contributor

ziofat commented Jun 16, 2023

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
Copy link
Contributor

isc30 commented Jun 16, 2023

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
Copy link
Contributor

ziofat commented Jun 16, 2023

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

@isc30
Copy link
Contributor

isc30 commented Jun 16, 2023

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
Copy link
Contributor

ziofat commented Jun 16, 2023

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

@ziofat
Copy link
Contributor

ziofat commented Jun 16, 2023

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

The first one is:

// Either it already satisfies the new version, or doesn't.
// If not, the downstream dep needs to be republished.
// The downstream dep will also need to be republished if using `workspace:*` as this will publish
// as the exact version.
changeType =
!isWorkspaceWildcardVersion &&
semver.satisfies(change.newVersion!, requiredVersion.versionSpecifier)
? ChangeType.dependency
: ChangeType.patch;

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

The second one is:

// Update the package's peer dependencies.
PublishUtilities._updateDependencies(
packageJson.name,
packageJson.peerDependencies,
allChanges,
allPackages,
rushConfiguration,
prereleaseToken,
projectsToExclude
);

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
Copy link
Contributor

isc30 commented Jun 19, 2023

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
Copy link
Contributor

ziofat commented Jun 20, 2023

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
Copy link
Contributor

ziofat commented Jun 20, 2023

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
Copy link
Contributor

isc30 commented Jun 20, 2023

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
Copy link
Contributor

ziofat commented Jun 21, 2023

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

@iclanton iclanton moved this to General Discussions in Bug Triage Aug 15, 2023
@ziofat
Copy link
Contributor

ziofat commented May 31, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement The issue is asking for a new feature or design change help wanted If you're looking to contribute, this issue is a good place to start! repro confirmed The issue comments included repro instructions, and the maintainers reproduced the problem rush-publish-requirements This issue describes user requirements that we should consider in the new design for "rush publish"
Projects
Status: General Discussions
Development

No branches or pull requests

6 participants