Skip to content
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-lib] Add pnpm-config.json to specify overrides,packageExtensions,neverBuiltDependencies in top level package.json created by Rush.js #3107

Merged
merged 20 commits into from
Sep 23, 2022

Conversation

chengcyber
Copy link
Contributor

@chengcyber chengcyber commented Dec 21, 2021

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.

@iclanton
Copy link
Member

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

@chengcyber
Copy link
Contributor Author

As per zkochan said in this comment pnpm/pnpm#4080 (comment), 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.

@iclanton
Copy link
Member

@octogonz - thoughts on where these options should live?

@chengcyber
Copy link
Contributor Author

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
Copy link
Contributor Author

chengcyber commented Apr 7, 2022

Hi @octogonz , i saw your issue at pnpm/pnpm#4407.

And zkochan adds more pnpm.xxx configurations into pnpm.

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

@iclanton
Copy link
Member

iclanton commented Apr 9, 2022

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.

@chengcyber chengcyber requested a review from patmill as a code owner April 9, 2022 07:30
@chengcyber chengcyber requested review from octogonz and dmichon-msft and removed request for halfnibble April 9, 2022 07:31
@dmichon-msft
Copy link
Contributor

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.

@chengcyber
Copy link
Contributor Author

chengcyber commented Jul 8, 2022

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 chengcyber changed the title [rush-lib] Add support for overrides,packageExtensions,neverBuiltDependencies in pnpmOptions [rush-lib] Add pnpm-config.json to specify overrides,packageExtensions,neverBuiltDependencies in top level package.json created by Rush.js Jul 11, 2022
Copy link
Contributor

@dmichon-msft dmichon-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like having Rush do any validation whatsoever of the pnpm field. That's pnpm's job and adding the extra validation layer just makes it more brittle.

chengcyber and others added 5 commits September 16, 2022 18:43
…-03.json

Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
…setting, and PNPM 5 is no longer supported as of Rush 5.47.0. Tune up the docs for the other "pnpmOptions" settings.
…ns", "neverBuiltDependencies", "allowedDeprecatedVersions", and a property bag "unsupportedPackageJsonSettings"
@chengcyber
Copy link
Contributor Author

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 chengcyber requested review from iclanton, octogonz and D4N14L and removed request for sachinjoseph, patmill, octogonz, iclanton and D4N14L September 19, 2022 11:34
chengcyber and others added 2 commits September 23, 2022 19:01
Co-authored-by: Ian Clanton-Thuon <iclanton@users.noreply.github.com>
@iclanton iclanton merged commit eeac262 into microsoft:main Sep 23, 2022
@chengcyber chengcyber deleted the feat-pnpm-options branch September 24, 2022 06:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants