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

chore: upgrade pnpm to version 9 #11637

Merged
merged 10 commits into from
May 28, 2024
Merged

Conversation

paoloricciuti
Copy link
Member

Svelte 5 rewrite

Feel free to close immediately if it's not desired but the upgrade should be seamless. I don't know if we should also specify the version in CI but let's see.

Also does this require a coordinated effort with sveltekit?

Please note that the Svelte codebase is currently being rewritten for Svelte 5. Changes should target Svelte 5, which lives on the default branch (main).

If your PR concerns Svelte 4 (including updates to svelte.dev.docs), please ensure the base branch is svelte-4 and not main.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with feat:, fix:, chore:, or docs:.
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests and linting

  • Run the tests with pnpm test and lint the project with pnpm lint

Copy link

changeset-bot bot commented May 15, 2024

⚠️ No Changeset found

Latest commit: 3be636a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@PuruVJ
Copy link
Collaborator

PuruVJ commented May 15, 2024

Ur a savior!

@Conduitry
Copy link
Member

As with sveltejs/kit#12129. I expect that this is blocked until Vercel supports pnpm v9.

@paoloricciuti
Copy link
Member Author

As with sveltejs/kit#12129. I expect that this is blocked until Vercel supports pnpm v9.

Can you approve the deployment to check if it's working? @PuruVJ said that he's currently deploying with pnpm 9

@Rich-Harris
Copy link
Member

I'll check in with the team, see if we can get an ETA on this

image

@PuruVJ
Copy link
Collaborator

PuruVJ commented May 15, 2024

Error is happening cuz of strictness of packageManager field, I am using pnpm 9 locally with lockfile v9, vercel has pnpm 8 but that supports lockfile 9 as well, so it works

@Rich-Harris
Copy link
Member

How do we fix it?

@PuruVJ
Copy link
Collaborator

PuruVJ commented May 15, 2024

Either wait for vercel to release v9, or remove packageManager field for now. Generate lockfile v9 from pnpm 9 locally, push it, and vercel's pnpm v8 will then use that v9 lockfile(it support that)

@Rich-Harris
Copy link
Member

Apparently Vercel already shipped pnpm 9, so maybe the problem is on our end?

@paoloricciuti
Copy link
Member Author

Apparently Vercel already shipped pnpm 9, so maybe the problem is on our end?

I think the problem is the stricness of the packageManager field? unfortunately you can't set it to something like pnpm@^9

@Rich-Harris
Copy link
Member

It seems there's a bug with the version detection — not really following the conversation internally but it could be what's causing these headaches

@paoloricciuti
Copy link
Member Author

It seems there's a bug with the version detection — not really following the conversation internally but it could be what's causing these headaches

Keep us posted 🤞🏼

@Rich-Harris Rich-Harris marked this pull request as draft May 15, 2024 16:54
@Rich-Harris
Copy link
Member

marking as draft for now to reduce noise

@paoloricciuti
Copy link
Member Author

marking as draft for now to reduce noise

Sure no prob 🤟🏻

@Rich-Harris
Copy link
Member

Looks like 9.0.4 (which I think is the latest available version) is good

@PuruVJ
Copy link
Collaborator

PuruVJ commented May 17, 2024 via email

@paoloricciuti
Copy link
Member Author

Looks like 9.0.4 (which I think is the latest available version) is good

9.1 is the latest but i think even if the packageManager is super strict once you have it locally you can use pnpm 9 even with higher version (at least it was like this with pnpm 8)

@Rich-Harris
Copy link
Member

Sorry, I meant the latest available version in the build environment. Though apparently this is due to some interaction with corepack — investigation ongoing

@benmccann
Copy link
Member

We'd also need to add an .npmrc file with package-manager-strict=false or else the project can't be run locally. Unless that default in pnpm 9 is reverted (pnpm/pnpm#8087)

@Rich-Harris
Copy link
Member

Update from @EndangeredMassa (thank you!) here: vercel/vercel#11607 (comment)

The relevant bit for us:

you can work around this problem by doing any of the following:

  • enable corepack (vercel docs)
    • set env var ENABLE_EXPERIMENTAL_COREPACK to 1
    • make sure you have a matching package.json#packageManager value
  • remove your package.json#engines.pnpm value (pnpm docs)

@benmccann
Copy link
Member

Awesome! Good to see the CI passing now

My comment above about needing the .npmrc will still need to be addressed or else people can't run this project locally without corepack anymore

@paoloricciuti
Copy link
Member Author

Awesome! Good to see the CI passing now

My comment above about needing the .npmrc will still need to be addressed or else people can't run this project locally without corepack anymore

If we remove engines we would get both the deployment on vercel and the ability to run locally right?

@benmccann
Copy link
Member

If we remove engines we would get both the deployment on vercel and the ability to run locally right?

No. That's how prior versions of pnpm worked, but pnpm 9 changed the behavior. I personally consider it to be a regression, but jury is still out on that: pnpm/pnpm#8087

package.json Outdated Show resolved Hide resolved
@benmccann
Copy link
Member

It turns out that the packageManager field is no longer required by Vercel. I removed it so that local builds work. Can you fix the merge conflicts in the lock file? Then I think we can merge this

@Conduitry
Copy link
Member

Without the packageManager field, though, aren't people with corepack not going to get the automatic version switching anymore? I don't think corepack works off engines field ranges. People would have to manually switch between versions of pnpm when different projects use different ones.

@benmccann
Copy link
Member

Yes, that's true, but we didn't put that there for contributors. It was there only for purposes of deploying to Vercel. It'll also stop the endless renovate PRs to bump that field whenever there's a new version of pnpm, which is another win from removing it

@paoloricciuti
Copy link
Member Author

It turns out that the packageManager field is no longer required by Vercel. I removed it so that local builds work. Can you fix the merge conflicts in the lock file? Then I think we can merge this

Fixing now...in favor of removing it

@benmccann
Copy link
Member

Hmm. Having to put with: version: 9 4 different places is kind of annoying though. Maybe we should wait a couple more days and see how the poll shakes out. Removing the package-manager-strict enforcement is only behind by half a dozen votes. I wonder if just setting that to false might be the better backup plan vs removing the packageManager field

@paoloricciuti
Copy link
Member Author

Hmm. Having to put with: version: 9 4 different places is kind of annoying though. Maybe we should wait a couple more days and see how the poll shakes out. Removing the package-manager-strict enforcement is only behind by half a dozen votes. I wonder if just setting that to false might be the better backup plan vs removing the packageManager field

It is quite annoying but also it only changes when a major of pnpm is released so it's not that often. Tbf the strictness of the packageManager field is kinda annoying on local too if you have a different version.

But up to you for the final decision 😄

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

I'll merge this for now. If pnpm/action-setup#126 gets merged then we can remove all the version: 9 stuff in a follow up

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