rushstack
rushstack copied to clipboard
[rush] Add an option to disable hoisting for indirect dependencies.
https://github.com/microsoft/rushstack/issues/3535 brought to light that Rush doesn't currently have a built-in way to disable hoisting of indirect dependencies with PNPM.
These .npmrc
options are used to disable any hoisting in PNPM, but they're off by default and are not particularly discoverable. I proposed adding these options to rush init
in https://github.com/microsoft/rushstack/pull/3541, but @octogonz brought up the concern that enabling these stricter options by default may also be frustrating and non-discoverable, especially for existing repos that are already relying on hoisting for indirect dependencies.
Instead of enabling those options in the common/config/rush/.npmrc
generated by rush init
, I'd like to propose adding an option to the pnpmOptions
section of rush.json
called strictIndirectDependencies
(although I'm totally open to alternative names). When this option is enabled, Rush would pass public-hoist-pattern=
, hoist=false
, and hoist-pattern=
to pnpm directly, instead of via the checked-in .npmrc
. This maximizes discoverability of the option and allows us to include a documentation comment in the rush.json
generated by rush init
.
enabling these stricter options by default may also be frustrating and non-discoverable, especially for existing repos that are already relying on hoisting for indirect dependencies.
As part of migrating a new monorepo, I've been thinking about various "strictness" settings. Some examples:
policy | tool | how Rush encourages it |
---|---|---|
link-workspace-packages=false |
PNPM | forced by rush install |
"--strict-peer-dependencies" | PNPM | opt-in via rush.json |
disable PNPM hoisting | PNPM | this issue #3542 |
ensureConsistentVersions | Rush | setting in rush.json |
Block node-gyp during rush install |
Rush | a future feature I'd like to propose 😉 |
tsconfig.json should set "types": [ ] | TypeScript | the default if you use Rush's rigs |
tsconfig.json should set skipLibCheck=false |
TypeScript | the default if you use Rush's rigs |
All the above settings have the following characteristics:
- The "strict" mode is not the default setting for the associated tool.
- But without the "strict" mode, it's more painful to maintain a large monorepo.
- If you try to enable it later, nontrivial work may be required to get to a clean state
That last point can be an adoption concern -- we wouldn't want someone to try migrating their monorepo to Rush, find that their projects fail to compile, and give up because they didn't realize they just needed to disable a "strict" setting. Sometimes it's better for the strict setting to default to disabled, but with a conspicuous doc comment recommending to try to get it enabled if possible.
This need for documentation is why I think these settings maybe are better formalized in rush.json rather than inviting users to tamper with common/config/rush/.npmrc
. The .npmrc
file is an unvalidatable property bag, so it seems better to treat that file as "use at your own risk."
The PNPM settings:
-
hoist-pattern
: "Tells pnpm which packages should be hoisted tonode_modules/.pnpm
. By default, all packages are hoisted - however, if you know that only some flawed packages have phantom dependencies, you can use this option to exclusively hoist the phantom dependencies (recommended)." defaults to:hoist-pattern[]=*
strict choice is:hoist-pattern[]=
-
hoist
: The docs imply thathoist=true
is a synonym forhoist-pattern[]=*
, but it's unclear what happens if both settings are specified together. -
public-hoist-pattern
: "Unlike hoist-pattern, which hoists dependencies to a hidden modules directory inside the virtual store,public-hoist-pattern
hoists dependencies matching the pattern to the root modules directory. Hoisting to the root modules directory means that application code will have access to phantom dependencies, even if they modify the resolution strategy improperly."
defaults to:public-hoist-pattern[]=*eslint*
andpublic-hoist-pattern[]=*prettier*
strict choice is:public-hoist-pattern[]=
-
shamefully-hoist
: The docs say thatshamefully-hoist=true
is a synonym forpublic-hoist-pattern[]=*
, but it's unclear what happens if both settings are specified together.
Seems like:
hoist-pattern |
public-hoist-pattern |
|
---|---|---|
affects dependencies of... | external packages | local workspace projects |
defaults to... | * |
ESLint and Prettier only |
likelihood of breaks in "strict" mode: | high risk | low risk |
synonym | hoist |
shamefully-hoist |
Does that sound right?
It took me a while to make sense of the synonyms. I agree we should try to simplify the settings for Rush. However it seems like disabling public-hoist-pattern
should be very easy, because the fixes are likely in source code that you control. Whereas disabling hoist-pattern
is more challenging (we accomplished it in the Rush Stack repo, but from what I hear Microsoft has not accomplished this yet in their internal monorepos).
Some questions for @dmichon-msft based on his experience with PR #3474 that enabled strictness in the Rush Stack repo:
-
For someone who is working incrementally towards strictness, how beneficial would it be to have
public-hoist-pattern[]=
strict, buthoist-pattern[]=*
? -
For someone who is working incrementally towards strictness, how beneficial would it be for
hoist-pattern
to specify some package names? Is this a quick solution for problems that would be difficult to work around usingpnpmfile.cjs
? Or would it be better to take an all-or-nothing approach, and avoid supporting patterns entirely?
A related observation: From the docs it seems that PNPM can pick up config from all sorts of unexpected places: ~/.npmrc
, /etc/npmrc
, environment variables, etc. This would be extremely bad for an important determining setting such as hoist-pattern
. To avoid accidents and ensure deterministic behavior, maybe rush install
should scan the PNPM config. For any PNPM settings that can be specified via a Rush config file, Rush should report an error if the PNPM configuration tries to specify them.
I'd like to propose adding an option to the
pnpmOptions
section ofrush.json
calledstrictIndirectDependencies
(although I'm totally open to alternative names). When this option is enabled, Rush would passpublic-hoist-pattern=
,hoist=false
, andhoist-pattern=
to pnpm directly, instead of via the checked-in.npmrc
.
Depending on answers to the questions above, we could consider some other variations of this design that allow more control:
-
Ian's design with PNPM config validation:
strictIndirectDependencies=true
in rush.json setspublic-hoist-pattern[]=
andhoist-pattern[]=
. In this mode, Rush scans the PNPM config✱ and reports an error if any of the four settings are configured anywhere. Due to this validation, there's no need for Rush to sethoist=false
. Whereas ifstrictIndirectDependencies=false
(the default value), thenhoist
andshamefully
hoist are forbidden in the PNPM config (to avoid confusion), butpublic-hoist-pattern
andhoist-pattern
are allowed; if omitted then the PNPM defaults apply. -
Separate switches:
strictIndirectDependencies=true
in rush.json controlshoist-pattern
as described above. But a separate settingallowShamefulHoisting
in rush.json controlspublic-hoist-pattern
: The default isallowShamefulHoisting=false
, which causes Rush to setpublic-hoist-pattern[]=
(even when strictIndirectDependencies=false). WithstrictIndirectDependencies=true
, Rush blockspublic-hoist-pattern
andshamefully-hoist
in the PNPM config.✱ Whereas withallowShamefulHoisting=true
then Rush does not setpublic-hoist-pattern
at all, and the validation is relaxed so thatpublic-hoist-pattern
andshamefully-hoist
can be set in.npmrc
. (If not, then the PNPM defaults apply.) As far as support, Rush'sallowShamefulHoisting=true
is "use it at your own risk." -
Separate switches with hoist patterns: If we think hoisting patterns will be commonly used, we could expose
hoistedIndirectDependencyPatterns
in rush.json (instead ofstrictIndirectDependencies
). This array would get mapped verbatim to the PNPMhoist-pattern
array. TheallowShamefulHoisting
setting could also be supported and would work as described in option 2.
✱ "PNPM config" refers to the effective config from user settings such as common/config/rush/.npmrc
, ~/.npmrc
, /etc/npmrc
, PNPM's environment variables, etc. "PNPM config" does NOT refer to the options passed by Rush when it invokes PNPM.
- For someone who is working incrementally towards strictness, how beneficial would it be to have
public-hoist-pattern[]=
strict, buthoist-pattern[]=*
?
Not at all. The effect of these settings are thus:
-
public-hoist-pattern
hoists modules tocommon/temp/node_modules
. -
hoist-pattern
hoists modules tocommon/temp/node_modules/.pnpm/node_modules
.
Since nothing looks for modules in common/temp
other than the packages that are stored inside of common/temp/node_modules/.pnpm
, both public-hoist-pattern
and hoist-pattern
have equivalent behavior for any given pattern.
- For someone who is working incrementally towards strictness, how beneficial would it be for
hoist-pattern
to specify some package names? Is this a quick solution for problems that would be difficult to work around usingpnpmfile.cjs
? Or would it be better to take an all-or-nothing approach, and avoid supporting patterns entirely?
Since changes to .pnpmfile.cjs
require a rush update --full
or similar to take effect, you can get a much faster inner loop for testing providing dependencies by enumerating exactly the set of phantom dependencies in your hoist-pattern[]=
entries. Once your entire repository builds successfully with only a specific allow list of hoisted dependencies in hoist-pattern[]=
, then you can add them to .pnpmfile.cjs
(or ideally to the pnpm.packageExtensions
setting in common/temp/package.json
so that you can do so declaratively: https://pnpm.io/package_json#pnpmpackageextensions).
Since Rush doesn't currently provide a way to specify the pnpm.packageExtensions
property, I haven't yet tested to see if changes in in will take effect with a normal rush update
, but if they do, that would definitely make it the superior code path.
Since nothing looks for modules in
common/temp
other than the packages that are stored inside ofcommon/temp/node_modules/.pnpm
, bothpublic-hoist-pattern
andhoist-pattern
have equivalent behavior for any given pattern.
Interesting! We should definitely explain that in the docs
@dmichon-msft Then does that mean the shamefully-hoist
behavior is incompatible with Rush?
- if
useWorkspaces=true
, thenshamefully-hoist
has no effect because the hoisting happens incommon/temp/node_modules
- if
useWorkspaces=false
, then it won't work due to the problems we previously encountered in PR #799
Based on David's feedback, here's another iteration:
- There's only one setting
hoistedIndirectDependencyPatterns
in rush.json. It defaults to["*"]
, but our doc comment will strongly encourage changing it to[ ]
. During installation Rush always setspublic-hoist-pattern[]=
. The four settingshoist-pattern
,public-hoist-pattern
,hoist
, andshamefully-hoist
are always forbidden in the user's PNPM config. ✱
The docs will explain that shamefully-hoist
and public-hoist-pattern
are not supported because they have no effect in Rush's installation model.