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

[rush] Add warning when using globalIgnoredOptionalDependencies in < pnpm 9.0.0 #5001

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

witcher112
Copy link
Contributor

Summary

"globalIgnoredOptionalDependencies" doesn't work if Rush monorepo is not configured with pnpm 9+.

Details

I've added mentions about it in the schema and a warning during rush install/update.

image

How it was tested

Impacted documentation

https://rushjs.io/pages/configs/pnpm-config_json/

Copy link
Member

@iclanton iclanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an error, not a warning. If someone configures something and that configuration isn't going to do anything, we usually error in that case.

@@ -71,6 +72,18 @@ export class InstallHelpers {
}

if (pnpmOptions.globalIgnoredOptionalDependencies) {
if (rushConfiguration.rushConfigurationJson.pnpmVersion !== undefined && semver.lt(rushConfiguration.rushConfigurationJson.pnpmVersion, '9.0.0')) {
// eslint-disable-next-line no-console
console.warn(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use terminal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that there's no terminal accessible in this scope. Should I rework the code a bit so it's passed down and accessible here or create a new instance of it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's easy enough to do, go for it, otherwise don't worry about it.

witcher112 and others added 2 commits November 15, 2024 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants