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] react to pnpm v7 local install path breaking change #3908

Merged
merged 6 commits into from
Jan 24, 2023

Conversation

jeremymeng
Copy link
Member

@jeremymeng jeremymeng commented Jan 19, 2023

Summary

In pnpm v7, the format of local installation paths and the hashing algorithm used to shorten paths have been changed. It caused error described in #3652. This PR adds handling logic for pnpm v7.

Details

How it was tested

The code is tested using https://github.com/Azure/azure-sdk-for-js repository

Impacted documentation

In pnpm v7, the format of local installation paths and the hashing algorithm
used to shorten paths have been changed. This PR add handling logic for pnpm v7.
@jeremymeng jeremymeng force-pushed the fix-pnpm-v7-local-path-3652 branch from 7dce97f to cac2db7 Compare January 19, 2023 22:08
@iclanton
Copy link
Member

Can we just use the dependency-path package instead of duplicating its functionality?

Use `depPathToFilename()` from `dependency-path` instead of duplicating code.
@jeremymeng
Copy link
Member Author

Can we just use the dependency-path package instead of duplicating its functionality?

@iclanton Good point. I made the change. As far as I know our repo doesn't have such cases that are impacted by the hasing change so I don't know how to test that. Also how do I test my PR in our repo? Previously I was just patching the js file under node_modules.

@iclanton
Copy link
Member

You can test this change in your repo by running node <path-to-rush-lib-in-your-clone-of-rushstack>/lib/start <args>, or, if you're testing something that uses the caching features, use node <path-to-rush-in-your-clone-of-rushstack>/lib/start-dev <args>.

@jeremymeng
Copy link
Member Author

You can test this change in your repo by running node <path-to-rush-lib-in-your-clone-of-rushstack>/lib/start <args>, or, if you're testing something that uses the caching features, use node <path-to-rush-in-your-clone-of-rushstack>/lib/start-dev <args>.

Thanks a lot! Tested with our repo on Linux/Windows.

@iclanton iclanton enabled auto-merge January 24, 2023 22:02
@iclanton iclanton merged commit 8e37354 into microsoft:main Jan 24, 2023
@jeremymeng jeremymeng deleted the fix-pnpm-v7-local-path-3652 branch January 24, 2023 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants