Open octogonz opened 1 month ago
I am excited to see this improvement! I would like to put some thoughts here:
require('relative_path/shared_pnpmfile.cjs')
?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.
The ensureConsistentVersions setting should be moved from rush.json to common-versions.json
strongly agree!
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?
- 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.
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
.)
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 runrush check
for this subpsace. Am I understanding it correctly?
Yes. I've clarified the wording.
@william2958 Here's how the docs update will look to reflect these changes:
Today @william2958 and I chatted about some remaining design questions for the Subspace config files, related to his PR https://github.com/microsoft/rushstack/pull/4715:
subspaces.md
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 withcommon/config/rush/.npmrc
The
subspaces/<name>/.pnpmfile-subspace.cjs
file (inherited from split workspaces) should get renamed to.pnpmfile.cjs
, since we forbidcomnmon/config/rush/.pnpmfile.cjs
when the feature is enabledcommon/config/rush/pnpm-config.json
should be treated as an all-or-nothing fallback in the case wheresubspaces/<name>/pnpm-config.json
is absent.ensureConsistentVersions
always applies within a subspace; there is no mode where it applies across all projects in the monorepoThe
ensureConsistentVersions
setting should be moved fromrush.json
tocommon-versions.json
so it is alongsideallowedAlternativeVersions
--subspace
is mandatory forrush check
when the feature is enabledrush version
,rush install
, etc always need to invokerush check
in a loop for each subspace withensureConsistentVersions=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