microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.82k stars 592 forks source link

[rush] Publish leaving shrinkwrap in stale state #3413

Open MichaelSitter opened 2 years ago

MichaelSitter commented 2 years ago

Summary

For publish runs using the apply & branch options which release workspace packages, the version bump edits to package.json files are not reflected in the shrinkwrap state. This will cause rush install to fail before running rush update. The workaround we are using is to omit the branch option to delay pushing the version bump commit and run rush update. but I'm a little hazy if this is safe in practice.

Please see zulip thread for original post: https://rushstack.zulipchat.com/#narrow/stream/262513-general/topic/publishing.20workspace.20dependencies.20create.20shrinkwrap.20conflict

Repro steps

  1. Setup a repo with two packages, packages A & B. Add package B as a workspace dependency in package A: "B": "workspace:~1.0.0"
  2. Create a change for package B.
  3. Run publish with the branch flag to commit the bump commit generated by apply: node common/scripts/install-run-rush.js publish --apply -b master --publish
  4. Re-clone or purge the repo and run rush install

Expected

Rush install should succeed after publish.

Actual

Install fails due to the lockfile not reflecting package.json version bumps.

The shrinkwrap file contains the following issues:
  Dependencies of project "<package>" do not match the current shrinkwrap.

The shrinkwrap file is out of date. You need to run "rush update".

Details

It's unclear to me from the publishing documentation if this expected behavior for publish. Our original release pipelines avoided this problem by publishing outside of the default branch, and required developers to get approvals to merge the branch with the publish bump commits. This is when they would run rush update and merge the results back to the default branch. But this introduced a bunch of ugly edge cases which proved difficult to address. Should publish just transparently include update?

Standard questions

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

Question Answer
@microsoft/rush globally installed version? 5.62.3
rushVersion from rush.json? 5.62.3
useWorkspaces from rush.json? yes
Operating system? mac
Would you consider contributing a PR? yes
Node.js version (node -v)? v14.17.6
dmichon-msft commented 2 years ago

The workaround you are using is, in fact, safe in practice, except insofar as the added latency allows more opportunities for merge conflicts in your branch. We'd accept a PR to fix this.

MichaelSitter commented 2 years ago

Thanks for getting back on this, we are getting someone from the team to start working on a patch to address this. Our current thinking was to make the behavior opt-in with a new publish flag, --update.

ndobryanskyy commented 2 years ago

We faced the similar issue, when started using workspace:^ specifiers instead of workspace:*. Rush started appending exact version ranges (numbers) after rush publish. So that in package.json { "@myorg/package-1:" "workspace:^" } had become { "@myorg/package-1:" "workspace:^x.y.z" } after first rush publish run. And when those numbers were also added in shrink-wrap we faced failing CI builds and merge conflicts on each PR.

Since we are using PNPM we were able to use .pnpmfile.cjs file to mitigate the problem and stop appending versions at least to shrink-wrap file. Posting minimal snippet below, hope that someone might find it useful.


function readPackage(pkg, context) {
 // ...

 if (pkg.name.startsWith('@myorg/')) { // This check is not strictly necessary, but it just avoids executing this for external packages
    fixWorkspaceDependenciesVersions(pkg.dependencies);
    fixWorkspaceDependenciesVersions(pkg.devDependencies);
    fixWorkspaceDependenciesVersions(pkg.peerDependencies);
  }
}

/**
 * Remove version numbers from workspace caret and tilde dependencies in the shrink-wrap file.
 * This prevents it from going stale after `rush publish`
 *
 * @example myDep: workspace:^x.y.z -> myDep: workspace:^
 *
 * @param dependenciesToFix dependencies map to fix
 */
function fixWorkspaceDependenciesVersions(dependenciesToFix) {
  const workspaceSpecifierToFixRegExp = /^workspace:[^~]/

  for (const [depName, depVersion] of Object.entries(dependenciesToFix || {})) {
    const versionMatch = depVersion.match(workspaceSpecifierToFixRegExp);
    if (!versionMatch) continue;

    dependenciesToFix[depName] = versionMatch[0];
  }
}
ndobryanskyy commented 2 years ago

Another interesting question to me is why Rush is writing to disk those exact version numbers during rush publish? Because as a user I would expect my package,json dependencies to remain untouched after publishing (like in workspace:* case), since I am using workspace version specifier.

I guess that this is the exact code piece that does it. And even from there it is apparent that star and caret/tilde ranges have different treatment.

Would be good to learn what is the rationale behind this, why is it necessary?

Maybe it is possible to stop writing exact numbers to disk (through package.json dependencies changes) during publish? That will also improve the overall maintainability when using ^ or ~ workspace ranges, because it will not be required to lookup exact latest version number of your workspace project when adding it as dependency (right now Rush will not allow having workspace:^ and workspace:^x.y.z by default failing rush install because of inconsistent versions)