[rush] Subspace Configuration Files #4442

william2958 · 2023-12-04T19:30:25Z

Summary

This MR introduces the configuration files for the upcoming subspaces feature, who's RFC is located here: https://github.com/microsoft/rushstack/blob/octogonz/rfc-4320-rush-subspaces/common/docs/rfcs/rfc-4230-rush-subspaces.md

This MR sets up:

The reading of the root level subspace.json configuration file for individual subspaces (As well as accompanying schema).
Modifications to project configurations in rush.json to support subspace specification.
Sets up the basics of reading subspace information in the rush-lib core library.

Details

How it was tested

Currently being manually tested

Impacted documentation

subspaces.json needs to be added to rush_files_and_folders.md on the website

common/reviews/api/rush-lib.api.md

libraries/rush-lib/src/api/SubspaceConfiguration.ts

libraries/rush-lib/src/schemas/subspace.schema.json

libraries/rush-lib/src/api/SubspaceConfiguration.ts

libraries/rush-lib/src/api/packageManager/PnpmPackageManager.ts

libraries/rush-lib/src/schemas/subspaces.schema.json

libraries/rush-lib/assets/rush-init/subspaces.json

libraries/rush-lib/src/api/RushConfiguration.ts

Co-authored-by: David Michon <[email protected]>

common/reviews/api/rush-lib.api.md

iclanton · 2023-12-06T23:22:50Z

libraries/rush-lib/assets/rush-init/subspaces.json

+   * This specifies whether or not to use the "subspaces" feature in rush. This allows grouping of
+   * projects into discrete workspaces known as "subspaces".
+   */
+  "enabled": false,


Should these properties have a /*[LINE "HYPOTHETICAL"]*/ prefix?

@octogonz?

Yes, if they are optional in the JSON schema.

But I think /*[LINE "HYPOTHETICAL"]*/ maybe should NOT be applied to the feature-gating setting ("useSubspaces"? "enabled"?) even if it is optional, since we want a casual observer to see that it is false without wondering what is the default value.

libraries/rush-lib/assets/rush-init/subspaces.json

common/reviews/api/rush-lib.api.md

libraries/rush-lib/src/api/RushConfiguration.ts

iclanton · 2023-12-07T02:26:22Z

libraries/rush-lib/src/api/RushConfiguration.ts

+    if (FileSystem.exists(subspaceConfigLocation)) {
+      // Try getting a subspace configuration
+      this.subspaceConfiguration = SubspaceConfiguration.loadFromConfigurationFile(subspaceConfigLocation);
+    }


Can you rename loadFromConfigurationFile to tryLoadFromConfigurationFile and just return undefined if the file doesn't exist?

And ideally this load should be async.

Changed to tryLoadFromConfigurationFile

Can this be made async?

(If the other similar Rush functions are not already async, then we shouldn't insist that Will fixes it in this PR.)

I'm a bit confused as to why this would be async - it only uses a call to FileSystem.exists, which as far as I can tell is a synchronous action, so if there are no asynchronous calls within the tryLoadFromConfigurationFile function, what purpose would labelling it async be?

There is a FileSystem.existsAsync, and there is a JsonFile.loadAndValidateAsync. Anything that touches the filesystem (including JSON file loading) should ideally be async to allow those calls to be parallelized.

libraries/rush-lib/src/api/RushConfiguration.ts

libraries/rush-lib/src/api/SubspaceConfiguration.ts

libraries/rush-lib/src/cli/actions/InitAction.ts

libraries/rush-lib/src/schemas/rush.schema.json

libraries/rush-lib/src/schemas/subspaces.schema.json

iclanton · 2023-12-07T02:40:59Z

libraries/rush-lib/src/schemas/subspaces.schema.json

+      "type": "boolean"
+    },
+    "splitWorkspaceCompatibility": {
+      "description": "(DEPRECATED) A depreciated option that allows individual subspaces to be configured at the package level if that package is the only project in the subspace. Used to help migrate from a split workspace state.",


We're introducing this with a deprecated feature? That doesn't seem right.

@iclanton This feature will be used internally by TikTok to incrementally migrate a huge monorepo that is using the earier #3481 branch. After the migration is complete, the setting will be removed. Because it is a company-specific concern, it seemed inappropriate to explain that in the schema comment.

octogonz · 2023-12-07T03:07:24Z

libraries/rush-lib/src/api/SubspaceConfiguration.ts

+    try {
+      resolvedSubspaceJsonFilename = trueCasePathSync(resolvedSubspaceJsonFilename);
+    } catch (error) {
+      /* ignore errors from true-case-path */


Discarding every possible error is always a suspicious practice.

Looking up the implementation in true-case-path/index.js, there are actually 3 separate errors that it can throw (aside from indirect errors such as Cannot read properties of undefined):

[true-case-path]: Called with ${filePath}, but no matching file exists

[true-case-path]: basePath argument must be absolute. Received "${basePath}"

[true-case-path]: filePath must be relative when used with basePath

The first one is probably what we're hoping to catch, whereas inadvertently catching the others would probably hide a bug.

I found /* ignore errors from true-case-path */ in two other places in the Rush code base, so probably @william2958 is simply copying that practice. Thus addressing this is out of scope for this PR.

But in the future maybe someone should move true-case-path into a library function and invent a more correct solution for nonexistent files.

@iclanton @dmichon-msft

common/changes/@microsoft/rush/will-intro-subspace_2023-12-04-16-28.json

Co-authored-by: Ian Clanton-Thuon <[email protected]>

…16-28.json Co-authored-by: Pete Gonzalez <[email protected]>

Co-authored-by: Ian Clanton-Thuon <[email protected]>

iclanton · 2023-12-07T21:20:12Z

libraries/rush-lib/src/cli/actions/InitAction.ts

@@ -185,7 +186,7 @@ export class InitAction extends BaseConfiglessRushAction {
      'rush.json'
    ];

-    const experimentalTemplateFilePaths: string[] = ['subspaces.json'];
+    const experimentalTemplateFilePaths: string[] = [`common/config/rush/${RushConstants.subspacesFilename}`];


Feels like the common/config/rush part should also be in a const (or consts?) somewhere.

That string already appears throughout this file. Seems like it's probably out of scope for this PR.

libraries/rush-lib/src/api/RushConfiguration.ts

common/reviews/api/rush-lib.api.md

libraries/rush-lib/src/api/RushConfiguration.ts

libraries/rush-lib/src/api/SubspaceConfiguration.ts

libraries/rush-lib/src/index.ts

libraries/rush-lib/src/api/SubspaceConfiguration.ts

libraries/rush-lib/src/logic/RushConstants.ts

Co-authored-by: Ian Clanton-Thuon <[email protected]>

william2958 added 5 commits December 1, 2023 11:05

feat: add subspace modifications to RushConfiguration

0239c70

feat: add subspace configuration file

b004a66

feat: add subspace configuration loadFromDefaultLocation

1bd9b71

chore: linting

d919129

chore: changefile

3b755ee

octogonz reviewed Dec 4, 2023

View reviewed changes

common/reviews/api/rush-lib.api.md Outdated Show resolved Hide resolved

octogonz reviewed Dec 4, 2023

View reviewed changes

libraries/rush-lib/src/api/SubspaceConfiguration.ts Outdated Show resolved Hide resolved

william2958 added 2 commits December 4, 2023 13:36

chore: fix documentation

2fbdc7d

chore: documentation and PR comments

2d53710

octogonz reviewed Dec 4, 2023

View reviewed changes

libraries/rush-lib/src/schemas/subspace.schema.json Outdated Show resolved Hide resolved

octogonz reviewed Dec 4, 2023

View reviewed changes

libraries/rush-lib/src/schemas/subspace.schema.json Outdated Show resolved Hide resolved

william2958 added 3 commits December 4, 2023 14:08

feat: add subspace json template to rush init

8f91e8a

chore: update description

d0370eb

feat: clean up loading of subspace configuration

c27c5ae

octogonz reviewed Dec 5, 2023

View reviewed changes

libraries/rush-lib/src/api/SubspaceConfiguration.ts Outdated Show resolved Hide resolved

octogonz reviewed Dec 5, 2023

View reviewed changes

libraries/rush-lib/src/api/packageManager/PnpmPackageManager.ts Outdated Show resolved Hide resolved

octogonz reviewed Dec 5, 2023

View reviewed changes

libraries/rush-lib/src/api/packageManager/PnpmPackageManager.ts Outdated Show resolved Hide resolved

octogonz reviewed Dec 5, 2023

View reviewed changes

libraries/rush-lib/src/schemas/subspaces.schema.json Outdated Show resolved Hide resolved

octogonz reviewed Dec 5, 2023

View reviewed changes

libraries/rush-lib/assets/rush-init/subspaces.json Outdated Show resolved Hide resolved

william2958 added 3 commits December 5, 2023 11:45

Clean up subspace configuration for PR

47e3e89

feat: update CommandLineHelp snapshot

8c8b539

chore: update rush-sdk snapshot

b7e1afd

william2958 marked this pull request as ready for review December 5, 2023 19:45

william2958 requested review from iclanton, apostolisms, D4N14L and dmichon-msft as code owners December 5, 2023 19:45

dmichon-msft reviewed Dec 5, 2023

View reviewed changes

libraries/rush-lib/src/api/RushConfiguration.ts Outdated Show resolved Hide resolved

dmichon-msft reviewed Dec 5, 2023

View reviewed changes

libraries/rush-lib/src/api/RushConfiguration.ts Outdated Show resolved Hide resolved

Update libraries/rush-lib/src/api/RushConfiguration.ts

8df0788

Co-authored-by: David Michon <[email protected]>

chore: syntax

bdf0c9e

iclanton changed the title ~~Subspace Configuration Files~~ [rush] Subspace Configuration Files Dec 6, 2023

iclanton reviewed Dec 6, 2023

View reviewed changes

common/reviews/api/rush-lib.api.md Outdated Show resolved Hide resolved

iclanton reviewed Dec 6, 2023

View reviewed changes

common/reviews/api/rush-lib.api.md Outdated Show resolved Hide resolved

chore: refactor to export subspace configuration interface

cf056e8

iclanton reviewed Dec 7, 2023

View reviewed changes

common/reviews/api/rush-lib.api.md Outdated Show resolved Hide resolved

common/reviews/api/rush-lib.api.md Outdated Show resolved Hide resolved

iclanton requested changes Dec 7, 2023

View reviewed changes

octogonz reviewed Dec 7, 2023

View reviewed changes

common/changes/@microsoft/rush/will-intro-subspace_2023-12-04-16-28.json Outdated Show resolved Hide resolved

william2958 and others added 10 commits December 7, 2023 11:07

Update libraries/rush-lib/assets/rush-init/subspaces.json

43b274f

Co-authored-by: Ian Clanton-Thuon <[email protected]>

Update libraries/rush-lib/src/api/RushConfiguration.ts

b5d58c6

Co-authored-by: Ian Clanton-Thuon <[email protected]>

Update libraries/rush-lib/src/api/RushConfiguration.ts

8f65295

Co-authored-by: Ian Clanton-Thuon <[email protected]>

Update libraries/rush-lib/src/api/SubspaceConfiguration.ts

8d42b59

Co-authored-by: Ian Clanton-Thuon <[email protected]>

Update common/changes/@microsoft/rush/will-intro-subspace_2023-12-04-…

d81881c

…16-28.json Co-authored-by: Pete Gonzalez <[email protected]>

Update libraries/rush-lib/src/schemas/rush.schema.json

bf31ea6

Co-authored-by: Ian Clanton-Thuon <[email protected]>

Update libraries/rush-lib/src/schemas/subspaces.schema.json

0aa57a4

Co-authored-by: Ian Clanton-Thuon <[email protected]>

PR comments

c8e85bf

PR comments

fa0c483

chore: add check to verify hydration of subspace names set

64f1628

iclanton reviewed Dec 7, 2023

View reviewed changes

libraries/rush-lib/src/api/RushConfiguration.ts Show resolved Hide resolved

rename _cachedRushProjectsBySubspaceName to _rushProjectsBySubspaceName

db32b88

iclanton reviewed Dec 8, 2023

View reviewed changes

william2958 and others added 4 commits December 7, 2023 20:03

Update libraries/rush-lib/src/api/RushConfiguration.ts

67c0c5f

Co-authored-by: Ian Clanton-Thuon <[email protected]>

Update libraries/rush-lib/src/api/RushConfiguration.ts

bf2cd20

Co-authored-by: Ian Clanton-Thuon <[email protected]>

PR comments

7184e21

Update some config file docs; mark new API's as "@beta"

f7f4cfd

octogonz approved these changes Dec 8, 2023

View reviewed changes

octogonz merged commit d391f10 into microsoft:main Dec 8, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rush] Subspace Configuration Files #4442

[rush] Subspace Configuration Files #4442

william2958 commented Dec 4, 2023 •

edited by octogonz

Loading

iclanton Dec 6, 2023

octogonz Dec 7, 2023

iclanton Dec 7, 2023

iclanton Dec 7, 2023

william2958 Dec 7, 2023

iclanton Dec 7, 2023

octogonz Dec 7, 2023

william2958 Dec 7, 2023

iclanton Dec 7, 2023

iclanton Dec 7, 2023

octogonz Dec 7, 2023

octogonz Dec 7, 2023 •

edited

Loading

iclanton Dec 7, 2023

octogonz Dec 7, 2023 •

edited

Loading

[rush] Subspace Configuration Files #4442

[rush] Subspace Configuration Files #4442

Conversation

william2958 commented Dec 4, 2023 • edited by octogonz Loading

Summary

Details

How it was tested

Impacted documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octogonz Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octogonz Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

william2958 commented Dec 4, 2023 •

edited by octogonz

Loading

octogonz Dec 7, 2023 •

edited

Loading

octogonz Dec 7, 2023 •

edited

Loading