[rush] rush-pnpm accepts patch and patch-commit

[chengcyber]

Summary

pnpm adds "pnpm patch" and "pnpm patch-commit" at 7.4.0. This PR makes rush-pnpm supports these two commands. Doc: https://pnpm.io/cli/patch

Details

1. Move logic in RushPnpmCommandLine to RushPnpmCommandLineParser
2. Make rush-pnpm accepts patch and patch-commit command
3. After running "pnpm patch-commit", copy "patches" folder to common/pnpm/patches and copy "common/temp/pnpm-lock.yaml" to "common/config/rush/pnpm-lock.yaml"
4. When rush install/update, sync common/pnpm/patches to common/temp/patches as well.

How it was tested

Using this demo repo https://github.com/chengcyber/rush-pnpm-patch-demo

[elliot-nelson]

@chengcyber This looks great!

I have pulled your branch and tested it locally and I don't seem to have it working, but I'm not sure why.

Here was my test (after pulling and building your branch of Rushstack locally):

$ cd tools/some-project
$ node ~/dev/rushstack2/libraries/rush-lib/lib/start-pnpm.js patch fast-glob
You can now edit the following folder: /private/var/folders/kk/ss84pygd5f55xycfvdndcxxc0000gp/T/27160b723efd47c7d03592b5480a4120/user

# at this point, i inserted some random console.log statements into fast-glob for testing

$ node ~/dev/rushstack2/libraries/rush-lib/lib/start-pnpm.js patch-commit /private/var/folders/kk/ss84pygd5f55xycfvdndcxxc0000gp/T/27160b723efd47c7d03592b5480a4120/user

 WARN  deprecated [email protected]: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.
 WARN  deprecated [email protected]: "Please update to latest v2.3 or v2.2"
../../common/temp                        |   +1 +
../../common/temp                        | Progress: resolved 220, reused 194, downloaded 0, added 1, done

dependencies:
- fast-glob 3.2.11
+ fast-glob 3.2.11
Copying "/Users/enelson/dev/Codex/common/temp/pnpm-lock.yaml"
  --> "/Users/enelson/dev/Codex/common/config/rush/pnpm-lock.yaml"
Rush refreshed the pnpm patch files in the "common/pnpm/patches" folder and shrinkwrap file.
  Please commit this change to Git.

$ rush install

After these steps, I tried running the tool but did not see my debug statements. So then I opened up the source in node_modules... i.e. vi node_modules/fast-glob/out/index.js... and my changes were not there.

The patch file (in common/pnpm/patches/[email protected]) looks correct, but it seems that rush install does not actually apply the patches as it installs. Does it work on your end?

[chengcyber]

Hi @elliot-nelson, Thanks to your feedback! I tested the feature in this repo https://github.com/chengcyber/rush-pnpm-patch-demo, and it works. Could you reproduce the issue in this repo. It aims to patch "[email protected]" to demonstrate the "pnpm patch" feature.

[elliot-nelson]

Hi @elliot-nelson, Thanks to your feedback! I tested the feature in this repo https://github.com/chengcyber/rush-pnpm-patch-demo, and it works. Could you reproduce the issue in this repo. It aims to patch "[email protected]" to demonstrate the "pnpm patch" feature.

Thanks for the demo repo link! I tried with your demo repo and still get a repro.

If I follow your steps exactly, then demo-a build log does have my console statement. But, if you run rush install, and then rebuild again, the console statement disappears... the rush install stomps out the patch that should be "baked in".

EDIT: That's the most important issue, but I thought I'd mention a couple other smaller items while I'm at it:

(Issue 2) In your demo repo you lay out how demo-a should be patched, but demo-b should not... that's not true after your changes in this PR, right? We would want that any use of [email protected] (even if it's nested deeply in an unsuspecting package) should be forced to use our patched version, and that it would be used across every package in the monorepo. I wasn't sure if your description of demo-a and demo-b was the desired state after these PR changes. (I do understand demo-c, it clearly would be unpatched.)

(Issue 3) If you were to accidentally attempt to use rush-pnpm patch with a pnpmVersion < 7.4.0, the behavior of pnpm is a little odd... instead of erroring out, it defaults to deciding patch is an existing system command and just running whatever patch is in your path. In my case that is /usr/bin/patch which just hangs forever waiting for input. Maybe we could dump out of the command immediately if you attempt to use rush-pnpm patch/patch-commit and pnpmVersion is too low, to prevent this potential confusion.

[chengcyber]

But, if you run rush install, and then rebuild again, the console statement disappears..

Thanks for the repro steps! I will take a look at it.

We would want that any use of [email protected] (even if it's nested deeply in an unsuspecting package) should be forced to use our patched version

For now, the existing behavior is only [email protected] in patched package like demo-a is "patched". Since this feature doesn't come from PNPM, I am not sure whether there is an easy implementation in Rush.js side.

Maybe we could dump out of the command immediately if you attempt to use rush-pnpm patch/patch-commit and pnpmVersion is too low, to prevent this potential confusion.

Nice catch!

[chengcyber]

But, if you run rush install, and then rebuild again, the console statement disappears..

After deep dive to the code, I found "pnpm patch-commit" not only modifing pnpm-lock.yaml but also modifing top level package.json with a pnpm.patchedDependencies configuration. e.g.

package.json:

{
  "pnpm": {
    "patchedDependencies": {
      "[email protected]": "patches/[email protected]"
    }
  }
}

This kind of configuration should be committed to Git as well. so, i believe i would like to implement it after this MR merged https://github.com/microsoft/rushstack/pull/3107, which have been approved 🤞

We would want that any use of [email protected] (even if it's nested deeply in an unsuspecting package) should be forced to use our patched version

I tried "patch-commit" in a normal pnpm workspace repo, it's not natively supported by pnpm. That means if run "pnpm patch-commit" under demo-a folder. The patched dependencies only affects demo-a, even though same version installed in demo-b.

[elliot-nelson]

@chengcyber Thanks for looking into those two issues!

On the first one, makes total sense, Rush will need to have logic to save that "patchedDependencies" section somewhere and copy it into the temporary package.json when it runs install (I guess).

On the second one, maybe we need to have more design... if you are running Rush with workspaces on and enforce consistent versions on, and you "patch" version 3.0.1 of sprintf-js (for example), I think a reasonable maintainer would assume that means all projects consuming [email protected] have been patched, and be very surprised if that's not true. So maybe (in a future PR) Rush can detect and fix this situation.

[chengcyber]

maybe we need to have more design... if you are running Rush with workspaces on and enforce consistent versions on

I totally agree with you. How about adding a --rush-make-consistent. As you know, it's the Rush.js takes responsible for "ensureConsistentVersion" instead of pnpm.

As a result, if user runs rush-pnpm patch-commit /path/to/patched_module, it patches the module only in the project folder as normal and logout a warning sth like "The pacthed module only works for project foo, if you want to patch for all projects with this module, you can specify with --rush-make-consistent".

And if user runs rush-pnpm patch-commit /path/to/patched_module --rush-make-consistent, rush figure out a list of projects and internally invoke "patch-commit" multiple times under respective project folders.

Does this make sense to you?

[chengcyber]

With https://github.com/microsoft/rushstack/pull/3107 has been merged, code changes were applied on the logic of rush-pnpm patch & patch-commit. All known issues should be fixed now.

Hi @elliot-nelson, could you kindly take another look?

[mathieukh]

We are looking for this feature too, is it possible to review this PR ?

[benkeen]

We're also waiting for this feature. Nice work @chengcyber, hope we can get a review soon.

[rafaelrozon]

Yes, it would be great to get this merged.

[elliot-nelson]

@chengcyber Sorry for the long delay, I've re-run all my tests and it looks great! Actually, it's working even better than I expected.

A quick rundown of my tests so we are on the same page:

1. Does the basic functionality work? - Yes. Using rush-pnpm patch and rush-pnpm patch-commit produces several common/config/ changes which can be committed; lines inserted into the target package execute as expected.
2. Is it sticky? (Does a patched package get used in other packages?) - Yes. Using rush add to add a patched package to another project in a repo automatically installs the expected version, with the patches.
3. Is it global? (Deep transitive dependencies are patched?) - Yes. Used rush-pnpm patch to patch jju (the json parser used by rush and heft) in a project, adding some console log spam. Running heft build in any project produces the console log spam, as expected.
4. Does it survive a rush install? - Yes. Switch between several branches and rush install, package is not patched. Checkout branch with pnpm patches and rush install, the package is patched again.

✅ In my opinion this feature is ready for prime time (and it looks like the git conflict is minor, so it should be quick to get it green as well).

The only test that did not work was this one:

5. Can you target any package? - No. For example, from the root of the monorepo, you can rush-pnpm patch any transitive dependency at any level of the lockfile, but when you try to apply that patch with rush-pnpm patch-commit, it will fail. Similarly, you can rush-pnpm patch any package from a project folder (even one that isn't in that package.json), but trying to apply the patch will fail.

(I don't believe this is a blocker, and maybe it's something that can be fixed in future. The result here is that if you want to "target" a package deep in your lockfile, you first need to add it to a package.json file somewhere in your monorepo to make it visible, then you can patch and apply the patch, and then remove it from that project.)

[chengcyber]

Thanks @elliot-nelson for the tests! I've rebased the branch from the latest main branch.

[octogonz]

:rocket: Released with Rush 5.86.0

[benkeen]

Thanks @chengcyber and everyone! Excited to give this a go. :)

[benkeen]

Just FYI, it's working very well @chengcyber! Just one little thing. I found that when I had a patch file already created, re-running rush-pnpm patch-commit with new changes had no effect. From the STDOUT it looks like it worked, but checking the generated patch file, there were no changes.

To get around it I just deleted the patch file + removed the entry in pnpm-config.json, then patch-commit generated the new patch file.

Perhaps this is consistent with the original pnpm command, I'm not sure. But if so, a warning to let the user know would be nice.

Thanks again!

[chengcyber]

Hi @benkeen, thanks for your valuable feedback! I can confirm this is a bug. The expected behavior is that new patched changes are synced to "common/pnpm-patches" when re-running rush-pnpm patch-commit. It will be fixed in https://github.com/microsoft/rushstack/pull/3791 as well