-
Notifications
You must be signed in to change notification settings - Fork 133
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
only use the Vercel config hack for versions that don't support bun #462
only use the Vercel config hack for versions that don't support bun #462
Conversation
🦋 Changeset detectedLatest commit: 256da09 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🧪 Prereleases are available for testing 🧪 @cloudflare/next-on-pagesYou can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/6226251465/npm-package-next-on-pages-462 @cloudflare/eslint-plugin-next-on-pagesYou can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/next-on-pages/runs/6226251465/npm-package-eslint-plugin-next-on-pages-462 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that Vercel added support for bun 🙂
(not too too suprising given the recent bun hype 😄)
But I think that this PR introduces a breaking change so unfortunately I don't think we can land it until the next major next-on-pages version...
The issue is that we currently allow people to use Vercel 30 and up your changes break this for all npm apps (because you update the peer dependencies and that if I remember correctly mismatched peer dependencies make the vercel command we run under the hood fail)
For bun applications specifically we actually even run process.exit(1)
, so before someone could have been using bun with an older version of the Vercel CLI and your hack but now if they update their next-on-pages version all of a sudden their build would fail...
So I am definitely afraid that this is indeed a breaking change that can break projects and that we need to apply on a major bump.... what do you think? 😢
Ah, I see, I don't use npm normally, so I wasn't aware of that. In that case I guess I should probably change the peer dep back to how it was before. Maybe this can be the time for me to finally get my wish and change it to
personally, i dont think this is major bump worthy at all. i view it as a fix since it's changing things to use the proper process of dealing with bun instead of a custom way. If that means someone needs to update their devdeps, they can update them. they have to update their devdeps to to use a new version of next-on-pages anyway. besides, none of that really matters. Bun support in Pages CI is only on a branch that you need to have your project opted into in the Discord if you want to help with testing it out, so there's only what, like 10 people at the maximum using Bun on Pages, and maybe half of them use Next.js (I'm one of them). If this is going to be a deal breaker for ya though, i guess we can keep the hack and only use it when the vercel version is less than 32.2.4. lmk and will be happy to make that change. |
mh... I still am not really sure about
ah well I wasn't following the bun support on Pages CI and didn't remember that it needs to be opted in... you do make a valid point.... however people could still have just npm installed bun in their build command right? 😕 anyways a breaking change is still a breaking change1... but I agree that it is very unlikely to affect many people, it could indeed likely fly under the radar without actually breaking any user... keeping the hack in would be the safest option.. but it's a bit cumbersome to keep that lying around until the next major bump... (especially since as we're discussing could not really be needed)... Let's then keep this a patch and I'll leave it up to you to decide if we want to keep the hack in or not (to be removed on our next major bump) 🙂 Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks a lot for the changes 🙂
Vercel recently introduced proper support for Bun as a package manager in vercel/vercel#10486 and vercel/vercel#10511 (after rejecting my PR claiming they had no plans to support it).
Because of this, it makes sense to remove our logic where we create a custom Vercel config to offer full support for Bun.
Since we are removing this logic, I have added a check to ensure that people are using a version of the Vercel CLI that supports Bun, so that older projects don't suddenly start using the wrong package manager.