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] Design note: Subspace configuration improvements #4720

Open
octogonz opened this issue May 17, 2024 · 5 comments
Open

[rush] Design note: Subspace configuration improvements #4720

octogonz opened this issue May 17, 2024 · 5 comments

Comments

@octogonz
Copy link
Collaborator

octogonz commented May 17, 2024

Today @william2958 and I chatted about some remaining design questions for the Subspace config files, related to his PR #4715:

subspaces.md
image

Here's a summary of the proposed changes:

  • The rush init template for .npmrc needs a # comment clearly explaining how the subspace .npmrc gets merged with common/config/rush/.npmrc

  • The subspaces/<name>/.pnpmfile-subspace.cjs file (inherited from split workspaces) should get renamed to .pnpmfile.cjs, since we forbid comnmon/config/rush/.pnpmfile.cjs when the feature is enabled

  • common/config/rush/pnpm-config.json should be treated as an all-or-nothing fallback in the case where subspaces/<name>/pnpm-config.json is absent.

    @octogonz will double-check with the maintainers whether this is intuitive enough or needs some clarification (such as subnspaces/_fallback/pnpm-config.json or common/config/rush/pnpm-config-fallback.json)

  • ensureConsistentVersions always applies within a subspace; there is no mode where it applies across all projects in the monorepo

  • The ensureConsistentVersions setting should be moved from rush.json to common-versions.json so it is alongside allowedAlternativeVersions

  • --subspace is mandatory for rush check when the feature is enabled

  • rush version, rush install, etc always need to invoke rush check in a loop for each subspace with ensureConsistentVersions=true (or if we are optimizing, for the subset of subspaces affected by that operation).

  • rush version should NOT support the --subspace parameter` since its operation isn't associated with any particular subspace

@iclanton @chengcyber

@chengcyber
Copy link
Contributor

I am excited to see this improvement! I would like to put some thoughts here:

  1. Is there any pattern to share code in subspace pnpmfile.js across different subspaces? If so, is it point to the JS fashion require('relative_path/shared_pnpmfile.cjs')?
  2. In the case where subspaces//pnpm-config.json is absent.

Is it say that the file-level absent? Or it means that property level absent in subspace's pnpm-config.json.

For example, useWorkspaces seems to be a monorepo-level configuration, and it cannot be set in subspace-level pnpm-config.json. Meanwhile, globalPnpmOverrides works great in subspace-level.

  1. The ensureConsistentVersions setting should be moved from rush.json to common-versions.json

strongly agree!

  1. rush version, rush install, etc always need to invoke rush check in a loop for every subspace (or else for a subset of subspaces affected by the operation).

If a certain subspace is ensureConsistentVersions disabled, it won't run rush check for this subpsace. Am I understanding it correctly?

@octogonz
Copy link
Collaborator Author

  1. Is there any pattern to share code in subspace pnpmfile.js across different subspaces? If so, is it point to the JS fashion require('relative_path/shared_pnpmfile.cjs')?

@william2958 seemed to think this was not a useful feature, at least in TikTok's monorepo where there are hundreds of subspaces. Although you could technically share logic using require('../../common-stuff'), Rush/PNPM has no way to detect changes to files referenced in that way, which will break the rush install analysis. Also, long term, I think we want to strongly discourage usage of .pnpmfile.cjs (code) in favor of pnpm-config.json (data) as it is easier to validate and cache.

@octogonz
Copy link
Collaborator Author

octogonz commented May 20, 2024

Is it say that the file-level absent? Or it means that property level absent in subspace's pnpm-config.json.

"in the case where subspaces/<name>/pnpm-config.json is absent" means the entire file is absent. "all-or-nothing fallback" means all fields or no fields.

For example, useWorkspaces seems to be a monorepo-level configuration, and it cannot be set in subspace-level pnpm-config.json. Meanwhile, globalPnpmOverrides works great in subspace-level.

The useWorksapces and subspacesEnabled settings are expected to be removed in Rush 6 (always on). In the latest Rush, subspacesEnabled will produce an error if useWorkspaces=false for any subspace, so #4720 means that if you create a subspace-level pnpm-config.json then you must set useWorkspaces=true in that file also.

(BTW it would be technically possible to support useWorkspaces=false with subspaces, and even allow true/false for different subspaces, but we don't want to invest in that as our goal is to eliminate useWorkspaces=false.)

@octogonz
Copy link
Collaborator Author

4. rush version, rush install, etc always need to invoke rush check in a loop for every subspace (or else for a subset of subspaces affected by the operation).

If a certain subspace is ensureConsistentVersions disabled, it won't run rush check for this subpsace. Am I understanding it correctly?

Yes. I've clarified the wording.

@octogonz
Copy link
Collaborator Author

@william2958 Here's how the docs update will look to reflect these changes:

microsoft/rushstack-websites#235

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

No branches or pull requests

2 participants