search

microsoft / rushstack

Monorepo for tools developed by the Rush Stack community
https://rushstack.io/
Other
5.82k stars 592 forks source link

[rush] Design Proposal: Replace PNPM indirect hoisting with generated dependencies in common/temp/package.json #3635

Open octogonz opened 1 year ago

octogonz commented 1 year ago

Summary

Problem: In Rush Hour East - Sep 2022 we discussed a problem report where a project failed to build after a partial/filtered install (rush install --to example-project), but the same project built correctly after a full install (rush install).

The cause was PNPM hoisting of indirect dependencies: With PNPM's default configuration (hoist-pattern: *), the hoisted set includes a version of every package, and any of these is potentially importable by a poorly behaved package. However PNPM partial install only hoists its own dependency tree (presumably because the full * is too many packages, which would undermine the savings of a partial install).

Solution: The ideal solution would be to disable hoisting entirely (the Rush Stack monorepo accomplished this in PR #3474). However eliminating hosting is challenging for most large monorepos, because many legacy NPM packages have lots of phantom dependencies, and determining the exact .pnpmfile.cjs fixes can be timeconsuming compared to hoisting.

@dmichon-msft proposed an intermediary solution:

  1. Disable hoisting (by removing hoist-pattern: * in .npmrc)
  2. Instead collect a small list of NPM packages the need to be hoisted, i.e. those packages that actually have problems; Rush should expose some setting to configure this list
  3. rush install should add these NPM packages to the "dependencies" field of the generated common/temp/package.json file

Compared to hoisting, the benefit is that version ranges can be specified explicitly, rather than relying on PNPM's heuristic.

This algorithm is similar to the old preferred versions behavior (with useWorkspaces=false).

CC @iclanton @chengcyber @elliot-nelson

iclanton commented 1 year ago

This sounds useful, and is definitely more clear behavior.

dmichon-msft commented 1 year ago

After some further investigation, using root package.json isn't quite right since it can lead to some complexity regarding satisfying peer dependencies of the hoisted packages. A better option would be to use the afterAllResolved pnpmfile hook to select from existing installed dependencies which ones should be hoisted in the . importer.