[rush-lib] ProjectChangeAnalyzer should analyze the scope of impact of pnpm-lock.yaml changes across all subspaces #5482
Conversation
…pm-lock.yaml changes
LPegasus
requested review from
D4N14L,
apostolisms,
dmichon-msft,
iclanton and
octogonz
as code owners
| }); | ||
| }); | ||
|
|
||
| describe.skip(ProjectChangeAnalyzer.prototype.getChangedProjectsAsync.name, () => { |
There was a problem hiding this comment.
This test is currently disabled
libraries/rush-lib/src/logic/test/ProjectChangeAnalyzer.test.ts
Outdated
Show resolved
Hide resolved
...ft/rush/fix-project-change-analyzer-nondefault-subspace-pnpmlock-check_2025-12-03-13-27.json
Outdated
Show resolved
Hide resolved
|
|
||
| const subspaces: ReadonlySet<Subspace> = rushConfiguration.subspacesFeatureEnabled | ||
| ? new Set(rushConfiguration.subspaces) | ||
| : new Set([rushConfiguration.defaultSubspace]); |
There was a problem hiding this comment.
If rushConfiguration.subspacesFeatureEnabled is false, it seems that rushConfiguration.subspaces will contain only the defaultSubspace?
rushstack/libraries/rush-lib/src/api/RushConfiguration.ts
Lines 893 to 902 in 262192e
There was a problem hiding this comment.
The code below seems to use subspaces only for iteration (Async.forEachAsync(subspaces, ...), so maybe it is not necessary to construct a Set?
There was a problem hiding this comment.
Yes. Here a Set is not needed. I'll change to use Array.
If rushConfiguration.subspacesFeatureEnabled is false, according to the code in https://github.com/LPegasus/rushstack/blob/1627a8386debc01fe198a2efcac0b8fa09fa5384/libraries/rush-lib/src/api/RushConfiguration.ts#L902
I think there's only one default subspace. Please let me know if I've overlooked any other possibilities that has other subspaces.
|
I added some stylistic feedback but otherwise LGTM. Thanks for adding tests! 🏅 |
| expect(changedProjects.has(rushConfiguration.getProjectByName(projectName)!)).toBe(true); | ||
| }); | ||
|
|
||
| // e depends on d via workspace:*, but its own package.json didn't change, so it's not included. e will be included by expandConsumers if needed. |
There was a problem hiding this comment.
Clarify that e is not included because the calculated lockfile footprint, i.e. .rush/temp/shrinkwrap-deps.json, for e did not change.
There was a problem hiding this comment.
Yes. Let me update the comment.
|
|
||
| module.exports = { | ||
| hooks: { | ||
| readPackage |
There was a problem hiding this comment.
This would seem to be invalid code? I don't see a definition of readPackage in this file.
There was a problem hiding this comment.
Oh, yes. Let me fix it.
…default-subspace-pnpmlock-check_2025-12-03-13-27.json Co-authored-by: Pete Gonzalez <4673363+octogonz@users.noreply.github.com>
Co-authored-by: Pete Gonzalez <4673363+octogonz@users.noreply.github.com>
…for subspaces iteration
1. Fix `.pnpmfile.cjs` `readPackage` function is not defined. 2. Fix the comment of why 'e' is not in changedProjects.
3 participants
Summary
Fixes #5480
Details
Q: How did you solve the problem?
A: Following the current logic of how to analyze default subspace
pnpm-lock.yamlchanges. Refactor the logic to run for all subspaces.Q: Mention any alternate approaches you considered.
A: I couldn't come up with a better alternative.
Q: Did you completely solve the problem, or are some cases not handled yet?
A: Yes. I think so.
Q: Does this change break backwards compatibility?
A: Yes.
Q: Could any aspects of your change impact performance?
A: For monorepo with many subspaces projects, it will take more time to check the pnpm-lock.yaml impact.
How it was tested
libraries/rush-lib/src/logic/test/repoWithSubspacesto test.rush list -t git:mainto check the listed projectsImpacted documentation
None