Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(rush): add support for resolution only mode with pnpm #4893

Merged

Conversation

aramissennyeydd
Copy link
Contributor

@aramissennyeydd aramissennyeydd commented Aug 16, 2024

Summary

Adding support for pnpm/pnpm#6411. This is required for enforcing strict peer dependency validation in CI. Otherwise, pnpm will skip the check if a lockfile exists (which it generally should for install). If these issues pass CI, local developers then see issues on rush add or rush update which recreates the lockfile and reruns the resolution checks.

Details

Adding support for the --resolution-only flag in PNPM. This flag force pnpm to re-run dependency resolution, https://pnpm.io/cli/install#--resolution-only. Without this flag, you end up in a situation similar to pnpm/pnpm#7087 and pnpm/pnpm#6893. With this flag, you can enforce this in CI with an additional rush install --resolution-only check.

One more caveat here (that I think is a bug with upstream PNPM) is that you can't set useFrozenLockfileForRushInstall to true and use the --resolution-only flag. It needs to be false otherwise you get a version mismatch error.

How it was tested

Tested against the main rush monorepo by removing the webpack allowed versions from globalPeerDependencyRules and verified that peer dependency errors do show up. Also tested against our internal rush monorepo

Impacted documentation

Rush CLI documentation.

common/config/rush/experiments.json Outdated Show resolved Hide resolved
libraries/rush-lib/src/cli/actions/InstallAction.ts Outdated Show resolved Hide resolved
@iclanton
Copy link
Member

@aramissennyeydd you'll probably need to update test snapshots

@aramissennyeydd
Copy link
Contributor Author

@iclanton Updated, also seeing this test case rushx should pass args to scripts failing both locally and in CI. Taking a look.

@iclanton iclanton enabled auto-merge (squash) August 19, 2024 20:06
@iclanton
Copy link
Member

iclanton commented Aug 21, 2024

It's actually this that's throwing:

await expect(async () => {
await Utilities.executeCommandAsync({
command: 'node',
args: [startPath],
workingDirectory: workingDir,
suppressOutput: true
});
}).not.toThrow();

Looks like that code actually has a small issue that I'm fixing here: #4895

@aramissennyeydd
Copy link
Contributor Author

@iclanton Interesting, thanks for taking a look into this! Just merged in master 🤞

@aramissennyeydd
Copy link
Contributor Author

@iclanton Looks like an issue here https://github.com/aramissennyeydd/rushstack/blob/0b87d488014e00c364bea509c238d4ab2f46300f/libraries/rush-lib/src/cli/actions/BaseRushAction.ts#L116-L118, the rush configuration can be undefined. I think that's a relatively large breaking change to adjust that, what's the best path forward here?

auto-merge was automatically disabled August 21, 2024 14:06

Head branch was pushed to by a user without write access

@iclanton
Copy link
Member

@aramissennyeydd - what you have for now is probably fine, but the type for rushConfiguration on BaseRushAction does appear to be wrong.

@iclanton
Copy link
Member

Let's merge this in, and then I can take a look at that issue separately (or you can, if you want 🙂).

@iclanton iclanton merged commit a277db6 into microsoft:main Aug 21, 2024
4 checks passed
@aramissennyeydd
Copy link
Contributor Author

@iclanton What would be the expected behavior here? If there's no rush config set, it looks like there's already a BaseConfiglessRushAction to handle that. I think I ran into trouble because I'm trying to access rushConfiguration in the constructor and its existence gets enforced in the onExecute.

@iclanton
Copy link
Member

I'm not sure offhand. I'd need to poke around more in the code. I'm currently largely away from a computer for the next few days (on vacation), but I may have some time to look later in the week or this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

2 participants