[rush] Support of pnpm patch command feature

[RIP21]

Will Rush work out of the box with this new pnpm patch https://pnpm.io/cli/patch feature or do you suspect it may break something?
I guess it will just work, but it won't patch it for all of the packages in the monorepo, but It's, possibly, maybe, intentional?

I don't have a Rush repo now under control to test it. But will be happy if this will be confirmed or specifically supported.

[elliot-nelson]

This is interesting to me as well. It looks like "pnpm patch" is just an easy way to set up to use the "patchedDependencies" field, right?

In our monorepo, one of our project teams had a deadline and resolved some emergent issues using a patch-package change on a package. As-is, this is not the end of the world; the package is currently only in use by their project. But if that changed and other projects used it, or if we began using some of the newer features like rush install --to <project>, we'd end up with potentially non-deterministic installs.

If there's a way today for me to take their patches/foo.patch file used by patch-package, and turn it into pnpm configuration in common/config/rush, that would definitely let me breathe easier (and provide an example way forward for teams that may need to patch a package in the future).

[RIP21]

@elliot-nelson it works perfectly with pnpm workspaces obviously. I guess since Rush uses the useWorkspaces, patch should work somehow? I mean at least there is a way to hack around it by calling for pnpm itself I think. But having it baked in Rush would be nice. patch-package doesn't work with pnpm obviously only yarn and pnpm.

Anyway, if somebody can try it and see how to get it working it would be nice. But as I said, I don't have a Rush repo now to test this quickly (and I'm lazy to create one for this thing alone :P)

[octogonz]

In our monorepo, one of our project teams had a deadline and resolved some emergent issues using a patch-package change on a package. As-is, this is not the end of the world; the package is currently only in use by their project. But if that changed and other projects used it, or if we began using some of the newer features like rush install --to <project>, we'd end up with potentially non-deterministic installs.

👎 patch-package is not a sound design -- it writes to another package's folder under node_modules disregarding package manager's storage integrity. For example, what if that package has multiple doppelgangers? Or what if I later checkout a different branch that is not using patch-package? How would the package manager know that the other package's folder needs to be reinstalled? patch-package seems like it will introduce nondeterminism that causes build failures that are difficult to repro.

pnpm patch is a much better solution because it enables PNPM to track the modifications and ensure integrity of node_modules.

If there's a way today for me to take their patches/foo.patch file used by patch-package, and turn it into pnpm configuration in common/config/rush, that would definitely let me breathe easier (and provide an example way forward for teams that may need to patch a package in the future).

For a production workspace, patching seems like a nontrivial user action that should be tracked carefully. For example, if you build libraries locally and publish them to NPM, their dependencies won't get patched when an external consumer installs the NPM package. This could cause confusion.

Maybe Rush could impose a little additional formalism. For example, maybe all patches should be stored under common/patches? Maybe the "patchedDependencies" setting could be specified via a config file such as common/patches/patches.json or common/config/pnpm.json that allows richer documentation/validation?

[elliot-nelson]

So, I did some testing of the pnpm patch and pnpm patch-commit commands today.

pnpm patch <pkg> is a command that works anywhere in any repo (including inside a Rush monorepo), as all it does is copy the target package's files into a temp folder and give you the path.

pnpm patch-commit <folder> is a two-step operation, first it configures the patch (by creating a patches/*.patch file and adding a patchedDependencies section to your package.json), and then it applies the patch (by running pnpm install which automatically patches as it installs).

The patch-commit configuration step is already compatible with Rush, in that it can write out the .patch file and edit the package.json file (either in a project, or in -C ../../common/temp). But, the following install step fails for various reasons and with various errors.

• Adding a patchedDependencies section to a project's package.json file fails because it cannot find the target package to patch (errors on various symlinks).
• Adding it to common/temp/package.json doesn't encounter the symlink issue but still can't find the target package to patch.

My conclusion so far is:

• There is no userland option today for taking advantage of pnpm patch-commit with Rush (this is a shame because patch-package does work out of the box with Rush, even though it works wrongly, as Pete pointed out. But that's a distinction that usually doesn't matter to the developers building software).
• Rush can probably just wrap pnpm patch, for the first step of creating a patch.
• Rush may or may not be able to take advantage of pnpm patch-commit command as-is to apply the patch (requires more investigation). Worst case, Rush may have to implement their own version of the same command to get the configuration in the right place. This isn't surprising given the way Rush separates installing and linking (I don't think this is compatible with the checks currently in place in for patchedDependencies within PNPM).

[RIP21]

@elliot-nelson have you used use Workspaces true while testing BTW?

[elliot-nelson]

@RIP21 ah, yes, this was all with useWorkspaces: true - I haven't tried it with that off.

[octogonz]

🎉 @chengcyber has created a PR: #3554

[chengjianhua]

This is a lifesaver! The PR to implement this has been merged. And I have experienced this feature, and it works for me.

It should be ok to close this issue and mark it resolved. 🎉