rushstack icon indicating copy to clipboard operation
rushstack copied to clipboard

[rush-lib] Add pnpm-config.json to specify overrides,packageExtensions,neverBuiltDependencies in top level package.json created by Rush.js

Open chengcyber opened this issue 3 years ago • 10 comments

Summary

Pnpm has some configurations in top level package.json. For now there is no way to add them. This PR adds support for overrides,packageExtensions,neverBuiltDependencies... in pnpmOptions

Details

Specify a bunch of pnpm configurations by pnpmOptions in rush.json. it will be added under common/temp/package.json's pnpm property.

  • overrides
  • packageExtensions
  • neverBuiltDependencies
  • onlyBuiltDependencies
  • peerDependencyRules

How it was tested

Test cases added, and tested in this repo.

chengcyber avatar Dec 21 '21 16:12 chengcyber

Should these options go in rush.json, or in a separate file? These options could potentially add a lot to rush.json?

iclanton avatar Dec 25 '21 06:12 iclanton

As per zkochan said in this comment https://github.com/pnpm/pnpm/issues/4080#issuecomment-991651063, these three properties are ok in monorepo scenario.

rush.json or a separate file?

I am not sure, but currently rush.json has pnpmOptions, so it makes sense to put them into rush.json.

chengcyber avatar Dec 25 '21 07:12 chengcyber

@octogonz - thoughts on where these options should live?

iclanton avatar Dec 25 '21 08:12 iclanton

Hi @octogonz , could you kindly take a look at this. I believe it helps a lot to fix tons of peer deps warnings in our monorepo.

chengcyber avatar Jan 14 '22 03:01 chengcyber

Hi @octogonz , i saw your issue at https://github.com/pnpm/pnpm/issues/4407.

And zkochan adds more pnpm.xxx configurations into pnpm.

  • peerDependencyRules https://github.com/pnpm/pnpm/pull/4212
  • onlyBuiltDependencies https://github.com/pnpm/pnpm/pull/4014

I'd like to add these fields into pnpmOptions in rush.json as well. In this way, repo maintainer can leverage full power of pnpm options easier.


Recap my proposal:

  • pnpm.xxx configurations can be specified in common/temp/package.json, currently by pnpmOptions.xxx in rush.json.(I have no objections if there is a better place)
  • this configurations natively works for the repo turn on useWorkspaces.(I don't pay much attention on useWorkspaces: false, since it seems it will be deprecated eventually?)
  • the order of precedence is overrides -> packageExtensions -> peerDependencyRules -> .pnpmfile.cjs readPackage (The later the higher)
  • the way to debug pnpm resolve results is rush install --debug-package-manager, it logs out a pnpm:progress log contains resolve status in ndjson format.

And, if the central way to manage pnpm options is in the opposite direction to Rush design goal. I am also open to close this PR. Then, the official way turn out to be specify them in each projects, which is acceptable but kind of difficult to use IMHO...

chengcyber avatar Apr 07 '22 16:04 chengcyber

Note: PR #3337 has renamed our GitHub master branch to main. This should not affect your pull request, but we recommend to redo your git clone to avoid confusion with the old branch name.

iclanton avatar Apr 09 '22 02:04 iclanton

Can we just put these options in a separate file, whose schema is not tracked by Rush (so that new options added to pnpm don't require updates to Rush to support), and whose contents just get directly transcribed into common/temp/package.json during the installation process? I'm not really a fan of polluting rush.json with this configuration, especially since it is likely to get fairly large in a large monorepo, and having separate files is better for git diffing and reviewing, as well as the issue with new options getting added to pnpm.

dmichon-msft avatar Jul 08 '22 00:07 dmichon-msft

Can we just put these options in a separate file, whose schema is not tracked by Rush (so that new options added to pnpm don't require updates to Rush to support), and whose contents just get directly transcribed into common/temp/package.json during the installation process? I'm not really a fan of polluting rush.json with this configuration, especially since it is likely to get fairly large in a large monorepo, and having separate files is better for git diffing and reviewing, as well as the issue with new options getting added to pnpm.

Sure! ~~Could you kindly point me out what's the best place to put this individual file? Iscommon/config/rush/pnpm-config.json good for you?~~

@dmichon-msft I updated my code, and now those fields are specified in common/config/rush/pnpm-config.json. Could you take another look?

chengcyber avatar Jul 08 '22 09:07 chengcyber

Update 2022/8/7:

  1. rebase the main branch
  2. Rush.js don't validate "pnpm.overrides", "pnpm.packageExtensions"... fields. Making the type to be unknown and leave pnpm to do the rest of work.

chengcyber avatar Aug 07 '22 13:08 chengcyber

No worries @iclanton, i think this kind of "back and forth" shows me how Rush maintainers think of a "small feature" which may affect the whole system. I recalled in the first implementation of this feature, from my previous experience, i choose to follow the fashion of making everything explicitly, which means Rush should validate every pnpm field by JSON Schema. But i didn't defend it well.

Now, here is a batch of code changes according to @octogonz 's proposal and your review suggestions. cc @dmichon-msft

Could you kindly take another look?

chengcyber avatar Sep 19 '22 11:09 chengcyber