[rush] Add an option to disable hoisting for indirect dependencies.

[iclanton]

trafficstars

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.

[octogonz]

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."

[octogonz]

The PNPM settings:

• hoist-pattern: "Tells pnpm which packages should be hoisted to node_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 that hoist=true is a synonym for hoist-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* and public-hoist-pattern[]=*prettier* strict choice is: public-hoist-pattern[]=

• shamefully-hoist: The docs say that shamefully-hoist=true is a synonym for public-hoist-pattern[]=*, but it's unclear what happens if both settings are specified together.

[octogonz]

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, but hoist-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 using pnpmfile.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.

[octogonz]

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.

Depending on answers to the questions above, we could consider some other variations of this design that allow more control:

1. Ian's design with PNPM config validation: strictIndirectDependencies=true in rush.json sets public-hoist-pattern[]= and hoist-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 set hoist=false. Whereas if strictIndirectDependencies=false (the default value), then hoist and shamefully hoist are forbidden in the PNPM config (to avoid confusion), but public-hoist-pattern and hoist-pattern are allowed; if omitted then the PNPM defaults apply.

2. Separate switches: strictIndirectDependencies=true in rush.json controls hoist-pattern as described above. But a separate setting allowShamefulHoisting in rush.json controls public-hoist-pattern: The default is allowShamefulHoisting=false, which causes Rush to set public-hoist-pattern[]= (even when strictIndirectDependencies=false). With strictIndirectDependencies=true, Rush blocks public-hoist-pattern and shamefully-hoist in the PNPM config.✱ Whereas with allowShamefulHoisting=true then Rush does not set public-hoist-pattern at all, and the validation is relaxed so that public-hoist-pattern and shamefully-hoist can be set in .npmrc. (If not, then the PNPM defaults apply.) As far as support, Rush's allowShamefulHoisting=true is "use it at your own risk."

3. Separate switches with hoist patterns: If we think hoisting patterns will be commonly used, we could expose hoistedIndirectDependencyPatterns in rush.json (instead of strictIndirectDependencies). This array would get mapped verbatim to the PNPM hoist-pattern array. The allowShamefulHoisting 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.

[dmichon-msft]

• For someone who is working incrementally towards strictness, how beneficial would it be to have public-hoist-pattern[]= strict, but hoist-pattern[]=*?

Not at all. The effect of these settings are thus:

• public-hoist-pattern hoists modules to common/temp/node_modules.
• hoist-pattern hoists modules to common/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 using pnpmfile.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.

[octogonz]

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.

Interesting! We should definitely explain that in the docs

[octogonz]

@dmichon-msft Then does that mean the shamefully-hoist behavior is incompatible with Rush?

• if useWorkspaces=true, then shamefully-hoist has no effect because the hoisting happens in common/temp/node_modules
• if useWorkspaces=false, then it won't work due to the problems we previously encountered in PR #799

[octogonz]

Based on David's feedback, here's another iteration:

4. 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 sets public-hoist-pattern[]=. The four settings hoist-pattern, public-hoist-pattern,hoist, and shamefully-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.