[rush] Fix rush pnpm patch commit#3791
Conversation
chengcyber
requested review from
D4N14L,
apostolisms,
iclanton and
octogonz
as code owners
|
Updates: Feedback at #3554 (comment), when re-running |
common/changes/@microsoft/rush/fix-rush-pnpm-patch-commit_2022-11-29-13-05.json
Outdated
Show resolved
Hide resolved
common/changes/@microsoft/rush/fix-rush-pnpm-patch-commit_2022-12-01-03-27.json
Outdated
Show resolved
Hide resolved
iclanton
force-pushed
the
fix-rush-pnpm-patch-commit
branch
from
de65df1 to
2d636cd
Compare
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
iclanton
force-pushed
the
fix-rush-pnpm-patch-commit
branch
from
2d636cd to
f80c1e6
Compare
| // patch-commit internally calls installation under cwd instead of the common/temp folder | ||
| // It throws missing package.json error, so in this case, we need to set the dir to the common/temp folder here | ||
| if (!pnpmArgs.includes('--dir') && !pnpmArgs.includes('-C')) { | ||
| if (!(await FileSystem.existsAsync(`${process.cwd()}/${FileConstants.PackageJson}`))) { |
There was a problem hiding this comment.
There are some repos that have a package.json in the root of a Rush repo, even though it's not considered by Rush/PNPM during install. Does this condition interfere with that scenario?
There was a problem hiding this comment.
I see that I mentioned this in my last round of reviews a couple years ago... I'm not sure where that landed in the comments though. Do we not need to care about this?
| // patch-commit internally calls installation under cwd instead of the common/temp folder | ||
| // It throws missing package.json error, so in this case, we need to set the dir to the common/temp folder here | ||
| if (!pnpmArgs.includes('--dir') && !pnpmArgs.includes('-C')) { | ||
| if (!(await FileSystem.existsAsync(`${process.cwd()}/${FileConstants.PackageJson}`))) { |
There was a problem hiding this comment.
I see that I mentioned this in my last round of reviews a couple years ago... I'm not sure where that landed in the comments though. Do we not need to care about this?
|
Today my colleague encountered the bug of not being able to update a patch previously generated by @chengcyber @iclanton Would you help to update this PR so it can be merged? Thanks a lot~ |
# Conflicts: # libraries/rush-lib/src/cli/RushPnpmCommandLineParser.ts
|
@kenrick95 I've merged |
6 participants
Summary
As @elliot-nelson pointed out at #3554 (comment),
rush-pnpm patch-commitfails when running it from the root of monorepo, this PR makesrush-pnpm patch-commitworks from the repo root folder.Details
The reason
rush-pnpm patch-commitfails is becausepnpm patch-commitinternally calls a logic which tries to read package.json file underprocss.cwd(). While the actual top-level package.json in Rush.js monorepo iscommon/temp/package.json. So when${cwd}/package.jsondoesn't exists and-Cor--dirCLI parameter is not specified, Rush internally specifies--dir common/tempparameter forrush-pnpm patch-commitHow it was tested
Tested it with this repo: https://github.com/chengcyber/rush-pnpm-patch-demo