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

Gate pnpm support behind a feature flag to avoid compatibility issues #1394

Merged
merged 1 commit into from
Jan 11, 2023

Conversation

charlespierce
Copy link
Contributor

Info

  • feat: Initial pnpm support #1273 Added support for the core pieces of pnpm: Instaling, pinning, and running.
  • Global package installation / interception are still TODO as we need to work out the specifics of how to implement those.
  • As noted in that PR, this change will break the behavior for anyone who was using pnpm as a global package in previous versions of Volta.
  • To maintain backwards compatibility, we want to make sure that those users aren't impacted by default.
  • We tackled a related issue previously with Yarn 3 support by adding a feature flag environment variable, which should work for this case as well.

Changes

  • Added a feature flag environment variable VOLTA_FEATURE_PNPM that will turn on the native pnpm support.
  • Updated the install, uninstall, run, and platform checkout flows to gate on the feature flag when handling pnpm
    • If the feature flag is off, we default to the package behavior, which is exactly how pnpm is handled in previous builds.
    • If the feature flag is on, then we use the new behavior that properly supports pnpm

Tested

  • Updated the tests to set the environment variable for any pnpm tests
  • Manually tested that with the feature flag off, things continue to treat pnpm as a global package
  • Manually tested that with the feature flag on, we use the pinned version of pnpm with the new behavior.
  • Confirmed that previous builds of Volta will still parse package.json if there is a pnpm value set - though it will be ignored since older versions don't know anything about that key.

Notes

  • Once pnpm support is completed with the global package controls, we will likely need some sort of migration to handle users that have pnpm installed as a global package and update their default platform with the appropriate pnpm version.
  • We will also remove the feature flag gating at that time and bump the minor version as it represents a new feature.

cc @chawyehsu Who did the initial pnpm implementation as well.

@smblee
Copy link

smblee commented Dec 19, 2022

When do we think this will be merged/released?

Copy link
Contributor

@mikrostew mikrostew left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

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

LGTM2!! 🚢

@charlespierce charlespierce merged commit bee4a53 into volta-cli:main Jan 11, 2023
@charlespierce charlespierce deleted the pnpm_feature_flag branch January 11, 2023 19:42
@chawyehsu
Copy link
Contributor

Great to see it gets merged. I guess for releasing the next release, there is only #1372 (#1372 (comment)) blocker left now?

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.

5 participants