-
Notifications
You must be signed in to change notification settings - Fork 607
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] Setting to avoid manually configuring decoupledLocalDependencies across subspaces #5072
Conversation
]) { | ||
const dependencySpecifier: DependencySpecifier = new DependencySpecifier(name, version); | ||
if ( | ||
rushConfiguration.rushConfigurationJson.projects.some((project) => project.packageName === name) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an O(n^2 * m)
algorithm cost, where n
is the # of projects in rush.json and m
is the number of dependencies in package.json. Additionally the [...
notation and projects.some(
notation will allocate lots of short-lived GC objects. Since this code runs for many Rush commands, I think it could be a performance concern.
rushConfiguration.rushConfigurationJson.projects.some((project) => project.packageName === name) && | ||
dependencySpecifier.specifierType !== DependencySpecifierType.Workspace | ||
) { | ||
this.decoupledLocalDependencies.add(name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we automatically generate decoupledLocalDependencies
for everything from devDependencies
and dependencies
, isn't that equivalent to disabling the policy entirely? Disabling the policy could be more cheaply accomplished by simply skipping the validation itself.
On the other hand, perhaps we DO want to enforce decoupledLocalDependencies
for some kinds of relationships but not others. For example, maybe decoupledLocalDependencies
should be enforced for dependencies within the same subspace, but not enforced for dependencies across subspaces? Or perhaps it is up to the library owner to declare whether they want decoupledLocalDependencies
to be enforced for dependencies on that library? (Maybe that is what you are already doing here?) Making the policy more configurable/flexible in this way would be great, but the rules need to be clearly explained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@octogonz
I agree with your point of view. decoupledLocalDependencies
can be declared within the subspace, and won't report errors for cross-subspace dependencies, but to achieve this goal, the following is required:
- A rush.json at the subspace level is required.
- Disable cross-subspace validation.
Given the current situation, the cost is relatively low, which allows us to quickly improve the user experience. Next, I think we can implement the solution we mentioned above to fully resolve this issue.
8b12ed8
to
c0a8d28
Compare
common/reviews/api/rush-lib.api.md
Outdated
@@ -481,6 +481,7 @@ export interface IExperimentsJson { | |||
buildCacheWithAllowWarningsInSuccessfulBuild?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/**
* Rush has a policy that normally requires Rush projects to specify `workspace:*` in package.json when depending
* on other projects in the workspace, unless they are explicitly declared as `decoupledLocalDependencies`
* in rush.json. Enabling this experiment will remove that requirement for dependencies belonging to a different
* subspace. This is useful for large product groups who work in separate subspaces and generally prefer to consume
* each other's packages via the NPM registry.
*/
/*[LINE "HYPOTHETICAL"]*/ "exemptDecoupledDependenciesBetweenSubspaces": false
if (referencedLocalProject && subspace.contains(referencedLocalProject)) { | ||
referencedLocalProject = undefined; | ||
} | ||
|
||
// Validate that local projects are referenced with workspace notation. If not, and it is not a | ||
// cyclic dependency, then it needs to be updated to specify `workspace:*` explicitly. Currently only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that this message is still referring to the old terminology "cyclic dependency" which we later renamed to "decoupled local dependency":
console.log(
Colorize.red(
`"${rushProject.packageName}" depends on package "${name}" (${version}) which exists ` +
'within the workspace but cannot be fulfilled with the specified version range. Either ' +
'specify a valid version range, or add the package as a cyclic dependency.'
)
);
Could you update that string to something like:
console.log(
Colorize.red(
`"${rushProject.packageName}" depends on package "${name}" (${version}) which belongs to ` +
'the workspace but cannot be fulfilled with the specified version range. Either ' +
'specify a valid version range, or add the package to "decoupledLocalDependencies" in rush.json.'
)
);
common/changes/@microsoft/rush/chore-auto-generate-decouple_2025-01-09-12-50.json
Outdated
Show resolved
Hide resolved
c5d5aba
to
62778aa
Compare
62778aa
to
59d7b98
Compare
Summary
Currently, our Monorepo contains numerous projects, and naturally, there are many projects that need to depend on other projects in the monorepo via version numbers rather than workspaces. As a result, business teams need to frequently modify the
rush.json
file.The purpose of
decoupledLocalDependencies
is to encourage projects in the repository to depend on each other through workspaces, which is understandable. However, as the number of projects in the monorepo increases, this becomes a burden for everyone. Therefore, there is a need for a capability to disable the configuration ofdecoupledLocalDependencies
.How it was tested
exemptDecoupledDependenciesBetweenSubspaces
totrue
.rush install
without any errors.