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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve support for PNPM 9 #9687

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

robaiken
Copy link
Contributor

@robaiken robaiken commented May 8, 2024

No description provided.

@robaiken robaiken changed the title Bump pnpm version to 9.1.0 Improve support for PNPM 9 May 8, 2024
@tusbar
Copy link
Contributor

tusbar commented May 10, 2024

@robaiken it seems that the corepack version is always the version getting used and the pnpm_version_numeric method doesn鈥檛 matter?

else
6
pnpm_version.to_i
Copy link

Choose a reason for hiding this comment

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

Much better! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

This assumes that the lockfile version will always match the pnpm version.
It is unsafe and probably wrong.

Copy link

Choose a reason for hiding this comment

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

OK, but that's the current state of things, right? This is a better version of what is currently already "unsafe and probably wrong", right?

Copy link

Choose a reason for hiding this comment

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

What's unsafe or wrong with this exactly ?

if pnpm_lockfile_version(pnpm_lock).to_f >= 9.0
  9
elsif pnpm_lockfile_version(pnpm_lock).to_f >= 6.0
  8
elsif pnpm_lockfile_version(pnpm_lock).to_f >= 5.4
  7
else
  6
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants