Add 'rush-pnpm up' support for catalogs#5585
Add 'rush-pnpm up' support for catalogs#5585benkeen wants to merge 25 commits intomicrosoft:mainfrom
Conversation
|
Howdy @iclanton, @dmichon-msft - don't suppose either of you have context or time to look at this? |
|
Shouldn't users be running |
|
Huh, neat. I never associated those two commands in my mind.
That could always be done with Tell me if I'm being barmy. |
|
Hey @iclanton - sorry to bug, got some time to look at this, this week? [We'd love this feature for our repo!] |
|
Hi @octogonz, @dmichon-msft would either of you be free for a review? |
iclanton
left a comment
There was a problem hiding this comment.
This overall looks fine, but would it be better to just include whatever functionality is missing from rush update?
I see what you're saying. I'm on the fence, but I think I'd lean towards keeping them separate. Feels like we'd be overloading Couple of thoughts:
I dunno. I'd say enhance the rush-pnpm command first, then depending on how it goes and what sort of demand is shown, consider elevating the functionality to the full rush update later. |
|
Updated, thanks @iclanton. |
iclanton
left a comment
There was a problem hiding this comment.
Left some comments about test cleanup and async-ifying things.
Otherwise I think the gist of this is fine.
libraries/rush-lib/src/cli/test/RushPnpmCommandLineParser.test.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/cli/test/RushPnpmCommandLineParser.test.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/cli/test/RushPnpmCommandLineParser.test.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/cli/test/RushPnpmCommandLineParser.test.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/cli/test/RushPnpmCommandLineParser.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…n.test.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…n.test.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…n.test.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…n.test.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…n.test.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…n.test.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…n.test.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…n.test.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…n.test.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…n.test.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…n.test.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…n.test.ts Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
|
Updated, thanks @iclanton. |
|
|
||
| describe('RushPnpmCommandLineParser', () => { | ||
| describe('catalog syncing', () => { | ||
| const testRepoPath: string = `${MONOREPO_ROOT}/temp/catalog-sync-test-repo`; |
There was a problem hiding this comment.
Don't do anything outside of the project folder. Do this in <projectRoot>/temp/catalog-sync-test-repo
| beforeEach(async () => { | ||
| await FileSystem.copyFilesAsync({ sourcePath: CATALOG_SYNC_REPO_PATH, destinationPath: testRepoPath }); | ||
|
|
||
| // common/temp is gitignored so it is not part of the static repo; copy the initial workspace file here |
There was a problem hiding this comment.
Can you add a .gitignore in the parent folder of with !temp?
| }); | ||
|
|
||
| it('does not update pnpm-config.json when catalogs are unchanged', async () => { | ||
| const { PnpmWorkspaceFile } = require('../../logic/pnpm/PnpmWorkspaceFile'); |
There was a problem hiding this comment.
| const { PnpmWorkspaceFile } = require('../../logic/pnpm/PnpmWorkspaceFile'); | |
| const { PnpmWorkspaceFile } = await import('../../logic/pnpm/PnpmWorkspaceFile'); |
| } | ||
| }); | ||
|
|
||
| const { PnpmWorkspaceFile } = require('../../logic/pnpm/PnpmWorkspaceFile'); |
There was a problem hiding this comment.
| const { PnpmWorkspaceFile } = require('../../logic/pnpm/PnpmWorkspaceFile'); | |
| const { PnpmWorkspaceFile } = await import('../../logic/pnpm/PnpmWorkspaceFile'); |
There was a problem hiding this comment.
Can you just import this at the top?
| `; | ||
| await FileSystem.writeFileAsync(pnpmWorkspacePath, workspaceWithoutCatalogs); | ||
|
|
||
| const { PnpmWorkspaceFile } = require('../../logic/pnpm/PnpmWorkspaceFile'); |
There was a problem hiding this comment.
| const { PnpmWorkspaceFile } = require('../../logic/pnpm/PnpmWorkspaceFile'); | |
| const { PnpmWorkspaceFile } = await import('../../logic/pnpm/PnpmWorkspaceFile'); |
| const MONOREPO_ROOT: string = path.dirname( | ||
| RushConfiguration.tryFindRushJsonLocation({ startingFolder: __dirname })! | ||
| ); | ||
| const TEST_TEMP_FOLDER: string = `${MONOREPO_ROOT}/temp/pnpm-config-update-test`; |
There was a problem hiding this comment.
Same here, don't do things outside of the project folder.
| }); | ||
|
|
||
| // Mock async version for loadCatalogsFromFileAsync | ||
| mockExistsAsync = jest.spyOn(FileSystem, 'existsAsync').mockImplementation(async () => { |
There was a problem hiding this comment.
Are we doing specific existence checks? We've moved to a convention to try to read and then catch the error and check if it's a not-exist error.
| if (this.jsonFilename) { | ||
| JsonFile.save(this._json, this.jsonFilename, { updateExistingFile: true }); | ||
| } | ||
| JsonFile.save(this._json, this.jsonFilename as string, { updateExistingFile: true }); |
There was a problem hiding this comment.
This will throw if jsonFilename is undefined. Is the property type wrong?
Addresses: #5578
Right now, when you update packages in a Rush/pnpm repo using
rush-pnpm up, it updates the catalog entries incommon/temp/pnpm-workspace.yaml, but doesn't update Rush's actual catalog, stored inpnpm-config.json, so the update command doesn't fully work. This has been supported since pnpm@10.12.0. This PR enhances it to update the pnpm-config file too.I also updated it to fix a small issue where JsonFile.save could throw an error due to an
undefined$schema property.Besides the tests, I've ran manual checks with our own Rush repo to confirm the catalog is getting updated properly.