-
Notifications
You must be signed in to change notification settings - Fork 594
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] Add an option to disable hoisting for indirect dependencies. #3542
Comments
As part of migrating a new monorepo, I've been thinking about various "strictness" settings. Some examples:
All the above settings have the following characteristics:
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 |
The PNPM settings:
|
Seems like:
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 Some questions for @dmichon-msft based on his experience with PR #3474 that enabled strictness in the Rush Stack repo:
A related observation: From the docs it seems that PNPM can pick up config from all sorts of unexpected places: |
Depending on answers to the questions above, we could consider some other variations of this design that allow more control:
✱ "PNPM config" refers to the effective config from user settings such as |
Not at all. The effect of these settings are thus:
Since nothing looks for modules in
Since changes to |
Interesting! We should definitely explain that in the docs |
@dmichon-msft Then does that mean the
|
Based on David's feedback, here's another iteration:
The docs will explain that |
#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 torush init
in #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 byrush init
, I'd like to propose adding an option to thepnpmOptions
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
. This maximizes discoverability of the option and allows us to include a documentation comment in therush.json
generated byrush init
.The text was updated successfully, but these errors were encountered: