Merge pull request #4700 from william2958/will/subspace-patch-24

[rush] Pass subspace parameter directly to SelectionParameterSet
microsoft · May 15, 2024 · 2fb00ea · 2fb00ea
2 parents 624dc0c + 740acf2
commit 2fb00ea
Show file tree

Hide file tree

Showing 9 changed files with 163 additions and 114 deletions.
diff --git a/common/changes/@microsoft/rush/will-subspace-patch-24_2024-05-13-20-35.json b/common/changes/@microsoft/rush/will-subspace-patch-24_2024-05-13-20-35.json
@@ -0,0 +1,10 @@
+{
+  "changes": [
+    {
+      "packageName": "@microsoft/rush",
+      "comment": "Fix an issue where the `--subspace` CLI parameter would install for all subspaces in a monorepo when passed to the install or update action",
+      "type": "none"
+    }
+  ],
+  "packageName": "@microsoft/rush"
+}
diff --git a/libraries/rush-lib/src/api/test/__snapshots__/RushCommandLine.test.ts.snap b/libraries/rush-lib/src/api/test/__snapshots__/RushCommandLine.test.ts.snap
@@ -361,14 +361,6 @@ Object {
           "required": false,
           "shortName": undefined,
         },
-        Object {
-          "description": "(EXPERIMENTAL) Specifies a Rush subspace to be installed. Requires the feature to be enabled in subspaces.json.",
-          "environmentVariable": undefined,
-          "kind": "String",
-          "longName": "--subspace",
-          "required": false,
-          "shortName": undefined,
-        },
         Object {
           "description": "Normally all projects in the monorepo will be processed; adding this parameter will instead select a subset of projects. Each \\"--to\\" parameter expands this selection to include PROJECT and all its dependencies. \\".\\" can be used as shorthand for the project in the current working directory. For details, refer to the website article \\"Selecting subsets of projects\\".",
           "environmentVariable": undefined,
@@ -433,6 +425,14 @@ Object {
           "required": false,
           "shortName": undefined,
         },
+        Object {
+          "description": "(EXPERIMENTAL) Specifies a Rush subspace to be installed. Requires the \\"subspacesEnabled\\" feature to be enabled in subspaces.json.",
+          "environmentVariable": undefined,
+          "kind": "String",
+          "longName": "--subspace",
+          "required": false,
+          "shortName": undefined,
+        },
         Object {
           "description": "Only check the validity of the shrinkwrap file without performing an install.",
           "environmentVariable": undefined,
@@ -868,14 +868,6 @@ Object {
           "required": false,
           "shortName": undefined,
         },
-        Object {
-          "description": "(EXPERIMENTAL) Specifies a Rush subspace to be installed. Requires the feature to be enabled in subspaces.json.",
-          "environmentVariable": undefined,
-          "kind": "String",
-          "longName": "--subspace",
-          "required": false,
-          "shortName": undefined,
-        },
         Object {
           "description": "Normally \\"rush update\\" tries to preserve your existing installed versions and only makes the minimum updates needed to satisfy the package.json files. This conservative approach prevents your PR from getting involved with package updates that are unrelated to your work. Use \\"--full\\" when you really want to update all dependencies to the latest SemVer-compatible version.  This should be done periodically by a person or robot whose role is to deal with potential upgrade regressions.",
           "environmentVariable": undefined,

diff --git a/libraries/rush-lib/src/cli/actions/BaseInstallAction.ts b/libraries/rush-lib/src/cli/actions/BaseInstallAction.ts
@@ -4,7 +4,6 @@
 import type {
   CommandLineFlagParameter,
   CommandLineIntegerParameter,
-  CommandLineStringParameter,
   IRequiredCommandLineIntegerParameter
 } from '@rushstack/ts-command-line';
 import { AlreadyReportedError } from '@rushstack/node-core-library';
@@ -46,7 +45,6 @@ export abstract class BaseInstallAction extends BaseRushAction {
   protected readonly _maxInstallAttempts: IRequiredCommandLineIntegerParameter;
   protected readonly _ignoreHooksParameter: CommandLineFlagParameter;
   protected readonly _offlineParameter: CommandLineFlagParameter;
-  protected readonly _subspaceParameter: CommandLineStringParameter;
   /*
    * Subclasses can initialize the _selectionParameters property in order for
    * the parameters to be written to the telemetry file
@@ -108,37 +106,13 @@ export abstract class BaseInstallAction extends BaseRushAction {
         ` if the necessary NPM packages cannot be obtained from the local cache.` +
         ` For details, see the documentation for PNPM's "--offline" parameter.`
     });
-    this._subspaceParameter = this.defineStringParameter({
-      parameterLongName: '--subspace',
-      argumentName: 'SUBSPACE_NAME',
-      description:
-        '(EXPERIMENTAL) Specifies a Rush subspace to be installed. Requires the feature to be enabled in subspaces.json.'
-    });
   }
 
-  protected abstract buildInstallOptionsAsync(): Promise<IInstallManagerOptions>;
-
-  protected getTargetSubspace(): Subspace {
-    const parameterValue: string | undefined = this._subspaceParameter.value;
-    if (parameterValue && !this.rushConfiguration.subspacesFeatureEnabled) {
-      // eslint-disable-next-line no-console
-      console.log();
-      // eslint-disable-next-line no-console
-      console.log(
-        Colorize.red(
-          `The "--subspace" parameter can only be passed if "subspacesEnabled" is set to true in subspaces.json.`
-        )
-      );
-      throw new AlreadyReportedError();
-    }
-    const selectedSubspace: Subspace | undefined = parameterValue
-      ? this.rushConfiguration.getSubspace(parameterValue)
-      : this.rushConfiguration.defaultSubspace;
-    return selectedSubspace;
-  }
+  protected abstract buildInstallOptionsAsync(): Promise<Omit<IInstallManagerOptions, 'subspace'>>;
 
   protected async runAsync(): Promise<void> {
-    const installManagerOptions: IInstallManagerOptions = await this.buildInstallOptionsAsync();
+    const installManagerOptions: Omit<IInstallManagerOptions, 'subspace'> =
+      await this.buildInstallOptionsAsync();
 
     if (this.rushConfiguration._hasVariantsField) {
       this._terminal.writeLine(
@@ -154,10 +128,32 @@ export abstract class BaseInstallAction extends BaseRushAction {
     let selectedSubspaces: Set<Subspace> | undefined;
     const subspaceInstallationDataBySubspace: Map<Subspace, ISubspaceInstallationData> = new Map();
     if (this.rushConfiguration.subspacesFeatureEnabled) {
-      selectedSubspaces = new Set();
+      // Selecting all subspaces if preventSelectingAllSubspaces is not enabled in subspaces.json
+      if (
+        this.rushConfiguration.subspacesConfiguration?.preventSelectingAllSubspaces &&
+        !this._selectionParameters?.didUserSelectAnything()
+      ) {
+        // eslint-disable-next-line no-console
+        console.log();
+        // eslint-disable-next-line no-console
+        console.log(
+          Colorize.red(
+            `The subspaces preventSelectingAllSubspaces configuration is enabled, which enforces installation for a specified set of subspace,` +
+              ` passed by the "--subspace" parameter or selected from targeted projects using any project selector.`
+          )
+        );
+        throw new AlreadyReportedError();
+      }
+
       const { selectedProjects } = installManagerOptions;
-      if (selectedProjects.size !== this.rushConfiguration.projects.length) {
-        // This is a filtered install. Go through each project, add its subspace's pnpm filter arguments
+
+      if (selectedProjects.size === this.rushConfiguration.projects.length) {
+        // Optimization for the common case, equivalent to the logic below
+        selectedSubspaces = new Set<Subspace>(this.rushConfiguration.subspaces);
+      } else {
+        selectedSubspaces = new Set();
+
+        // This may involve filtered installs. Go through each project, add its subspace's pnpm filter arguments
         for (const project of selectedProjects) {
           const { subspace: projectSubspace } = project;
           let subspaceInstallationData: ISubspaceInstallationData | undefined =
@@ -183,26 +179,6 @@ export abstract class BaseInstallAction extends BaseRushAction {
             pnpmFilterArgumentValues.push(project.packageName);
           }
         }
-      } else if (this._subspaceParameter.value) {
-        // Selecting a single subspace
-        const selectedSubspace: Subspace = this.rushConfiguration.getSubspace(this._subspaceParameter.value);
-        selectedSubspaces = new Set<Subspace>([selectedSubspace]);
-      } else {
-        // Selecting all subspaces if preventSelectingAllSubspaces is not enabled in subspaces.json
-        if (!this.rushConfiguration.subspacesConfiguration?.preventSelectingAllSubspaces) {
-          selectedSubspaces = new Set<Subspace>(this.rushConfiguration.subspaces);
-        } else {
-          // eslint-disable-next-line no-console
-          console.log();
-          // eslint-disable-next-line no-console
-          console.log(
-            Colorize.red(
-              `The subspaces preventSelectingAllSubspaces configuration is enabled, which enforces installation for a specified set of subspace,` +
-                ` passed by the "--subspace" parameter or selected from targeted projects using any project selector.`
-            )
-          );
-          throw new AlreadyReportedError();
-        }
       }
     }
 
@@ -299,7 +275,11 @@ export abstract class BaseInstallAction extends BaseRushAction {
           await this._doInstall(installManagerFactoryModule, purgeManager, installManagerOptionsForInstall);
         }
       } else {
-        await this._doInstall(installManagerFactoryModule, purgeManager, installManagerOptions);
+        // Simple case when subspacesFeatureEnabled=false
+        await this._doInstall(installManagerFactoryModule, purgeManager, {
+          ...installManagerOptions,
+          subspace: this.rushConfiguration.defaultSubspace
+        });
       }
     } catch (error) {
       installSuccessful = false;
@@ -352,7 +332,7 @@ export abstract class BaseInstallAction extends BaseRushAction {
 
   private _collectTelemetry(
     stopwatch: Stopwatch,
-    installManagerOptions: IInstallManagerOptions,
+    installManagerOptions: Omit<IInstallManagerOptions, 'subspace'>,
     success: boolean
   ): void {
     if (this.parser.telemetry) {

diff --git a/libraries/rush-lib/src/cli/actions/InstallAction.ts b/libraries/rush-lib/src/cli/actions/InstallAction.ts
@@ -31,11 +31,14 @@ export class InstallAction extends BaseInstallAction {
     });
 
     this._selectionParameters = new SelectionParameterSet(this.rushConfiguration, this, {
-      // Include lockfile processing since this expands the selection, and we need to select
-      // at least the same projects selected with the same query to "rush build"
-      includeExternalDependencies: true,
-      // Disable filtering because rush-project.json is riggable and therefore may not be available
-      enableFiltering: false
+      gitOptions: {
+        // Include lockfile processing since this expands the selection, and we need to select
+        // at least the same projects selected with the same query to "rush build"
+        includeExternalDependencies: true,
+        // Disable filtering because rush-project.json is riggable and therefore may not be available
+        enableFiltering: false
+      },
+      includeSubspaceSelector: true
     });
 
     this._checkOnlyParameter = this.defineFlagParameter({
@@ -44,7 +47,7 @@ export class InstallAction extends BaseInstallAction {
     });
   }
 
-  protected async buildInstallOptionsAsync(): Promise<IInstallManagerOptions> {
+  protected async buildInstallOptionsAsync(): Promise<Omit<IInstallManagerOptions, 'subspace'>> {
     const selectedProjects: Set<RushConfigurationProject> =
       (await this._selectionParameters?.getSelectedProjectsAsync(this._terminal)) ??
       new Set(this.rushConfiguration.projects);
@@ -68,7 +71,6 @@ export class InstallAction extends BaseInstallAction {
       pnpmFilterArgumentValues:
         (await this._selectionParameters?.getPnpmFilterArgumentValuesAsync(this._terminal)) ?? [],
       checkOnly: this._checkOnlyParameter.value,
-      subspace: this.getTargetSubspace(),
       beforeInstallAsync: () => this.rushSession.hooks.beforeInstall.promise(this),
       terminal: this._terminal
     };

diff --git a/libraries/rush-lib/src/cli/actions/ListAction.ts b/libraries/rush-lib/src/cli/actions/ListAction.ts
@@ -109,11 +109,14 @@ export class ListAction extends BaseRushAction {
     });
 
     this._selectionParameters = new SelectionParameterSet(this.rushConfiguration, this, {
-      // Include lockfile processing since this expands the selection, and we need to select
-      // at least the same projects selected with the same query to "rush build"
-      includeExternalDependencies: true,
-      // Disable filtering because rush-project.json is riggable and therefore may not be available
-      enableFiltering: false
+      gitOptions: {
+        // Include lockfile processing since this expands the selection, and we need to select
+        // at least the same projects selected with the same query to "rush build"
+        includeExternalDependencies: true,
+        // Disable filtering because rush-project.json is riggable and therefore may not be available
+        enableFiltering: false
+      },
+      includeSubspaceSelector: false
     });
   }
 

diff --git a/libraries/rush-lib/src/cli/actions/UpdateAction.ts b/libraries/rush-lib/src/cli/actions/UpdateAction.ts
@@ -36,11 +36,14 @@ export class UpdateAction extends BaseInstallAction {
     if (this.rushConfiguration?.subspacesFeatureEnabled) {
       // Partial update is supported only when subspaces is enabled.
       this._selectionParameters = new SelectionParameterSet(this.rushConfiguration, this, {
-        // Include lockfile processing since this expands the selection, and we need to select
-        // at least the same projects selected with the same query to "rush build"
-        includeExternalDependencies: true,
-        // Disable filtering because rush-project.json is riggable and therefore may not be available
-        enableFiltering: false
+        gitOptions: {
+          // Include lockfile processing since this expands the selection, and we need to select
+          // at least the same projects selected with the same query to "rush build"
+          includeExternalDependencies: true,
+          // Disable filtering because rush-project.json is riggable and therefore may not be available
+          enableFiltering: false
+        },
+        includeSubspaceSelector: true
       });
     }
 
@@ -75,7 +78,7 @@ export class UpdateAction extends BaseInstallAction {
     return super.runAsync();
   }
 
-  protected async buildInstallOptionsAsync(): Promise<IInstallManagerOptions> {
+  protected async buildInstallOptionsAsync(): Promise<Omit<IInstallManagerOptions, 'subspace'>> {
     const selectedProjects: Set<RushConfigurationProject> =
       (await this._selectionParameters?.getSelectedProjectsAsync(this._terminal)) ??
       new Set(this.rushConfiguration.projects);
@@ -99,8 +102,6 @@ export class UpdateAction extends BaseInstallAction {
       pnpmFilterArgumentValues:
         (await this._selectionParameters?.getPnpmFilterArgumentValuesAsync(this._terminal)) ?? [],
       checkOnly: false,
-      subspace: this.getTargetSubspace(),
-
       beforeInstallAsync: () => this.rushSession.hooks.beforeInstall.promise(this),
       terminal: this._terminal
     };