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] Enable support for PNPM "--resolution-strategy fewer-dependencies" #1180

Closed
octogonz opened this issue Mar 25, 2019 · 11 comments
Closed
Labels
effort: easy Probably a quick fix. Want to contribute? :-) enhancement The issue is asking for a new feature or design change help wanted If you're looking to contribute, this issue is a good place to start!

Comments

@octogonz
Copy link
Collaborator

PNPM 3.1 introduces a new command-line option --resolution-strategy fewer-dependencies that changes the version solver to be more like the NPM behavior. Among other things, this option eliminates a problem where sometimes @types dependencies get the wrong version installed. (Fundamentally, we believe DefinitelyTyped is relying too much on NPM idiosyncrasies, and needs to improve their approach. But until then, it's very helpful for PNPM to use a more NPM-compatible algorithm.) The full story can be found in the pnpm/pnpm#1187 discussion.

Proposal:

  1. Add a rush.json option to configure the --resolution-strategy option for PNPM.
  2. Make fewer-dependencies the default setting.
@octogonz octogonz added enhancement The issue is asking for a new feature or design change help wanted If you're looking to contribute, this issue is a good place to start! effort: easy Probably a quick fix. Want to contribute? :-) labels Mar 25, 2019
@tnc1997
Copy link
Contributor

tnc1997 commented Mar 30, 2019

Hi @octogonz! Hope you don't mind, but I thought I'd have a go at making the relevant changes to the rush-lib app in a fork based on your proposal. I'm more than happy to open up a Pull Request if you like.

@octogonz
Copy link
Collaborator Author

Awesome! If I remember right this option was introduced only for PNPM 3, which has changed the filename for shrinkwrap.yaml - did you encounter that issue?

@tnc1997
Copy link
Contributor

tnc1997 commented Mar 30, 2019

I didn't hit any issues running both rush update and rush rebuild using "pnpmVersion": "3.1.0-0", which is the version that is mentioned in pnpm/pnpm#1187 (comment), but on reflection I have noticed two 'lock' files in the temp directory: shrinkwrap.yaml and pnpm-lock.yaml (pnpm/pnpm#1738). Admittedly that was using "rushVersion": "5.6.1" in the configuration though. My apologies, but I'm not 100% sure how I would go about using the locally changed rush-lib app to update and rebuild the repository.

@octogonz
Copy link
Collaborator Author

For a quick fix, I think you pretty much just need to edit RushConstants.ts

  /**
   * The filename ("shrinkwrap.yaml") used to store state for pnpm
   */
  export const pnpmShrinkwrapFilename: string = 'shrinkwrap.yaml';

The complete fix is tracked by #1145 and probably would select the name based on the PNPM version.

@tnc1997
Copy link
Contributor

tnc1997 commented Mar 30, 2019

Cheers for the pointer in the right direction, much appreciated. I've modified that constant as below:

  /**
   * The filename ("pnpm-lock.yaml") used to store state for pnpm
   */
  export const pnpmShrinkwrapFilename: string = 'pnpm-lock.yaml';

I'll update the fork, but as you say it'll need further work so as not to break backwards compatibility.

@octogonz
Copy link
Collaborator Author

Any early feedback you could provide would be appreciated - does it work, were there any other issues, etc.

@tnc1997
Copy link
Contributor

tnc1997 commented Mar 30, 2019

Of course, so using "rushVersion": "5.6.1" and "pnpmVersion": "3.1.0-0" appeared to work fine without any issues when performing a rush update and a rush rebuild earlier, however the caveat of course is that duplicate 'lock' files are produced as mentioned in the first change.

I've subsequently tested with the second change in place, by overwriting .rush\node-v10.15.3\rush-5.6.1\node_modules\@microsoft\rush-lib with the built web-build-tools\apps\rush-lib, and both rush update and rush rebuild executed successfully whilst only producing the appropriate pnpm-lock.yaml files (no shrinkwrap.yaml files) in web-build-tools\common\config\rush and web-build-tools\common\temp.

The only thing that I haven't had a chance to fully test yet, is to add the resolutionStrategy option to the rush.json file and use the different values available.

I am more than happy to help out further with this issue wherever I can, so please do feel free to let me know if you could do with additional feedback etc.

@octogonz
Copy link
Collaborator Author

octogonz commented Mar 31, 2019

One thing to keep in mind is the file copying performed by rush install and rush update. There are 3 files:

A. common/config/rush/shrinkwrap.yaml - the "official" version that is tracked by Git
B. common/temp/shrinkwrap.yaml - the version that PNPM sees when invoked in common/temp
C. common/temp/shrinkwrap-preinstall.yaml - a backup copy of shrinkwrap.yaml, for diagnostic purposes to compare with B

rush install reads A, makes some fixes to it, and writes out B and C before invoking pnpm install

pnpm install reads B and might rewrite it with some changes, making it different from C. (But by design, rush install does not copy these changes back to A; the deviations are allowable because they are supposed to be fully deterministic. The reason is to minimize churn that causes Git merge conflicts.)

rush update is like rush install but it does copy B -> A, but only when Rush determines that the changes are interesting.

rush update --full deletes B, then runs pnpm install to create B, then copies B -> A

@tnc1997
Copy link
Contributor

tnc1997 commented Mar 31, 2019

Thanks for the heads up, it's very much appreciated! I've just been running rush update --full with the two different resolutionStrategy options in rush.json this morning and have attached the pnpm-lock.yaml files which are output each time to the web-build-tools\common\config\rush directory.

@bsiegel
Copy link
Member

bsiegel commented May 14, 2019

Is this completed by #1210?

@octogonz
Copy link
Collaborator Author

Yup :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: easy Probably a quick fix. Want to contribute? :-) enhancement The issue is asking for a new feature or design change help wanted If you're looking to contribute, this issue is a good place to start!
Projects
None yet
Development

No branches or pull requests

3 participants