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

fix vercel deployments version mismatch #544

Closed
wants to merge 3 commits into from
Closed

Conversation

technophile-04
Copy link
Collaborator

Description

Changes to flow :

1 . Very First time deployment :

The only change is you need to hit extra enter because of the extra first question "No Project Settings found locally....." :

Screen.Recording.2023-09-26.at.9.59.42.AM.1.mov

2 . Deployment to prod or getting preview llink :

  1. Instead of running "yarn vercel --prod" now "yarn vercel" by default deploys to prod
  2. For preview deployments we need to run yarn vercel:preview

3 . vercel:yolo :

No changes to this

Other options explored :

I was actually trying to get #231 (comment) work by people without needing to specify "packages/nextjs" and tried with different option configuring in vercel.json but nothing worked :(

Also tried reaching out on Next discord checkout our discusstion thread, summary :

  • we didn't find any config option to configure "code location" in vercel.json
  • Different approach : Using Vercel REST API for uploading and deployments (lol not feasible at all)

Drawback and Advantages of current approach :

Advantages :

  1. Its really superfast as compared to uploaidng and then building on vercel server
  2. Its solves our problem where local versions != deployed versions

Drawback(which I could think of ) :

  1. To run plain vercel commands now you need to run yarn vercel-cli
  2. Maybe on the very first deployment, first question "No Project Settings found locally.....Run vercel pull.." might confuse the user about what they need to
  3. When doing yarn vercel:preview vercel logs this at the end :
    "📝 To deploy to production (test-test-test-test-pi.vercel.app), run vercel --prod"
    this might give the wrong notion to users to use vercel --prod where instead they need to do just yarn vercel for prod deployments

Also in case if we go with this please feel free to suggest the script names, I overrided "vercel" since people are habitat for running yarn vercel to deploy app directly

we could also do "vercel:ship" etch please feel free to suggest 🙌

@technophile-04 technophile-04 linked an issue Sep 26, 2023 that may be closed by this pull request
1 task
@rin-st
Copy link
Member

rin-st commented Sep 26, 2023

I was actually trying to get #231 (comment) work by people without needing to specify "packages/nextjs" and tried with different option configuring in vercel.json but nothing worked :(

Tried it on weekends too, same result.

When I found vercel build and vercel deploy --prebuilt commands I thought: nah, I don't want to do two steps. I didn't think that I can run both at the same script 🤦‍♂️ . Good job figuring it out!

Instead of running "yarn vercel --prod" now "yarn vercel" by default deploys to prod

Good point. As I understand preview mode is good only for teams

Maybe on the very first deployment, first question "No Project Settings found locally.....Run vercel pull.." might confuse the user about what they need to

We can (and need to) add it to readme

When doing yarn vercel:preview vercel logs this at the end :
"📝 To deploy to production (test-test-test-test-pi.vercel.app), run vercel --prod"
this might give the wrong notion to users to use vercel --prod where instead they need to do just yarn vercel for prod deployments

This is not very good. As an option we can add && node -e \"console.log('🏗 Explore Scaffold-ETH-2 documentation for deploy options 🏗')\" or something like that to our deploy scripts. In terminal it will look like
image

To sum up: Looks like a decent solution for deployment problem! Great job as always!

@carletex
Copy link
Member

Thanks for testing this out @technophile-04!

As I understand by looking at the changed code, this prebuilds the projects and then deploys (so it uses the yarn.lock file), right?. It's a cool solution, but will only solve when deploying from the cli (I guess most people do that? I always do it by connecting the GitHub repo from the Vercel UI). Also, some of the prompts are a bit confusing. I like the default --prod tho.

Ideally, we'd want to solve this issue completely. I guess right now it's pnpm vs. locking versions on package.json... which also have their drawbacks.

What do you guys think?

@technophile-04
Copy link
Collaborator Author

I always do it by connecting the GitHub repo from the Vercel UI

But this does not have any problem, right? Because vercel UI auto-detects its monorepo and also auto-detects where "nextjs" project is right (it considers root yarn.lock during deployment) :

Screen.Recording.2023-09-27.at.2.02.12.PM.mov

Umm I think I have some misunderstandings on why we really need pnpm(which allows shared locked files) OR lock version in package.json now (would really appreciate it if someone could clarify).

Summarizing what I understand currently :

main issue : "Version mismatch between the local & prod env" i.e. #419

The reason: We are not able to ship monorepo root yarn.lock to prod env which is vercel in our case. we are only shipping nextjs dir (only happens when you do it through CLI)

The "main issue" only happens when deploying through CLI because connecting through GH vercel uses root yarn.lock.

So I think this PR solves the "main issue" :

  • using CLI : we prebuild locally (which uses root yarn.lock) and then deploy
  • connecting through GH is already sorted

Now, what are the added benefits of pnpm(shared .lock files) OR locking versions in package.json apart from solving the deployments issue(since its already solved if we merge this PR) ?

  1. Locking versions in package.json :
    Umm it feels weird that we have locked version in package.json even though we have yarn.lock already present.

  2. pnpm(shread .lock per pacakge) :
    Upgrade to pnpm #444 seems a bit unstable to me not because we are using pnpm but because we are using its shared lock file per workspace feature.
    While I was doing research for Upgrade to pnpm #444 I actually didn't find any good examples which were using this feature. Also I personally felt while raising the issue on pnpm that it does not focuses on this feature more and treats it kind of just an extra experimental add-on (We are also seeing slow install times as compared to normal pnpm because of this share .lock files their as I understand )

Regarding our CLI branch:

  • Locking versions in package.json makes sense there since we don't have .lock files and up for it 🙌 (We already using ~ their instead of ^)

  • Not sure about adding .lock files by default. I mean currently its fine because we just have plain SE-2 next but when add extensions like TheGrpah then we will be needing to add more packages to plain SE-2 next which might not be easy

This are my understanding and I might be completely wrong at some places and would really appreciate if someone could clarify 🙏

@carletex
Copy link
Member

Hey @technophile-04! Thank you so much for the write-up & context. Now I see everything more clearly. Some of the stuff you mentioned was on my notes (I was planning to post it in the pnpm PR, but will do it here)

I had a wall of text haha, but since you already did a great job given all the context, let me just comment on it and propose next steps.

main issue : "Version mismatch between the local & prod env" i.e. #419

Exactly. This is the most «critical» issue. We thought that having separate lock files would fix it (which it will!), but It was weird to me that not so many people were using that feature (and is not present in other package managers) & it seemed a bit unstable.

This PR fixes the issue (you are 100% right, it's not an issue when connecting repo in Vercel UI) in a better way 🚀

CLI

Locking versions in package.json makes sense there since we don't have .lock files and up for it 🙌 (We already using ~ their instead of ^)
Not sure about adding .lock files by default. I mean currently its fine because we just have plain SE-2 next but when add extensions like TheGrpah then we will be needing to add more packages to plain SE-2 next which might not be easy

Agree with what you said. We can lock versions in package.json (or ~)... and yeah, it can get messy pre-generating .lock files (as you said, extensions might add some deps to other extensions). Maybe, if we want to super-optimize, we could generate a couple of global .lock files (nextjs + hardhat / nextjs + foundry). It'd improve performance for the most used option (but this is future future haha)

so...

PNPM

If I remember correctly the main reason we started thinking about PNPM was the issue that this PR is fixing:

Also just mentioning pnpm here 😅 (It's workspaces has this option to create separate lock files) ....I tried it out and its really good also we might also benefit from it using it in our CLI due to its some what faster installations

(Shiv on #419 (comment))

Yes, I get it, pnpm is fancy and faster (especially if we disabled the per-package lock files) but I don't think it's worth it at this point. It has some issues on Windows (the env vars), and IT'S DAMN HARD TO WRITE, READ AND SAY haha

So now I change my 51% (pnpm) / 49% (yarn) vote to 1% (pnpm) / 99% (yarn)... and I'd keep an eye on Bun. It could be a nice option to improve performance. BTW, I think yarn gets a lot of shit, but I think v3 is good & fast enough.

Thoughts?

@carletex carletex mentioned this pull request Sep 27, 2023
@Pabl0cks
Copy link
Collaborator

Awesome job @technophile-04 @rin-st @carletex on these deep thoughts and researching, was nice to read it and try to learn from you!!

I've just tried this on Windows, and it's not working exactly like on Mac. The yarn vercel flow is not as smooth, asking to run vercel pull --yes command to retrieve project settings, and stopping the yarn vercel workflow.

After some noob tries 😅 (will need to explain it well in the Readme 🙌), and finally pulling the project settings, yarn vercel worked great and did the local deploy + vercel deploy perfectly.

Here is an screenshot:

image

@rin-st
Copy link
Member

rin-st commented Sep 27, 2023

Regarding our CLI branch:
Locking versions in package.json makes sense there since we don't have .lock files and up for it 🙌 (We already using ~ their instead of ^)
Not sure about adding .lock files by default. I mean currently its fine because we just have plain SE-2 next but when add extensions like TheGrpah then we will be needing to add more packages to plain SE-2 next which might not be easy

Agree 👍

Yes, I get it, pnpm is fancy and faster (especially if we disabled the per-package lock files) but I don't think it's worth it at this point.

For me main reason is that it's basically breaking change. Users got used to yarn, all the articles/videos use yarn etc. How to write and say it, I don't care. Probably because I don't speak about it 😄 .
Second reason is much smaller but as Shiv already said, it's much slower with shared lock files.

I have experience working with it, and it's good. It's fast and saves memory on disk. But yes, it's not worth it in our case

So now I change my 51% (pnpm) / 49% (yarn) vote to 1% (pnpm) / 99% (yarn)

Agree!

and I'd keep an eye on Bun

I tried it. For now, it's not so good for monorepos, for example you can't use scripts from your workspaces in main package.json. But as I understand they're adding features fast so I'm planning to check how it works from time to time


And again, if we're deciding to merge this, I believe we should also add info about it to readme/docs

@technophile-04
Copy link
Collaborator Author

Maybe, if we want to super-optimize, we could generate a couple of global .lock files (nextjs + hardhat / nextjs + foundry)

😆 trust me this same thing came to my mind after writing the comment, love this idea !!

I'd keep an eye on Bun.
I tried it. For now, it's not so good for monorepos, for example you can't use scripts from your workspaces in main package.json. But as I understand they're adding features fast so I'm planning to check how it works from time to time

Yup yup, I think incase in the future if we plan to change package manage bun might be better options hopefully bun will get more stable by then

Also btw in CLI since we are not adding .lock file we could let people choose the packageManager of their choic (we have yarn patch but we can generate similar patches in the corresponding package manager and ship respective which user chose) lol again this way ahead in future


Regarding this PR it seems this is not working as intended on windows :(

reason: As shown in SS #544 (comment) first yarn vercel ideally Vercel CLI should have prompted Pablo to select [Y/n] but rather it just exited the command.

I have created an issue on Vercel -> vercel/vercel#10614

Also while debugging with Pablo on TG chat I noticed Vercel CLI behaves bit different on Windows it rather exits directly instead of prompting (which it does on macOS atleast)

So I am leaning towards removing ^ from package.json for now

Maybe since #541 was solved we could wait over this weekend to see if Vercel replies if not we can close this PR and even #444 (feeling sad saying this 🥲 lol no JK 😂)

And we can make a PR removing ^ from package.json next week 🙌, what do you all think ?

@carletex
Copy link
Member

Also btw in CLI since we are not adding .lock file we could let people choose the packageManager of their choic (we have yarn patch but we can generate similar patches in the corresponding package manager and ship respective which user chose) lol again this way ahead in future

This is a good idea! (and I think people will like it too). It might be tricky a bit tricky for the docs (when running the commands):

<your_package_manager> run deploy

haha. Anyway, we can think about this in future iterations

I tried it. For now, it's not so good for monorepos, for example you can't use scripts from your workspaces in main package.json. But as I understand they're adding features fast so I'm planning to check how it works from time to time
Yup yup, I think incase in the future if we plan to change package manage bun might be better options hopefully bun will get more stable by then

Yes, let's keep an eye on it and see how it evolves. We can rethink in a few months.

How to write and say it, I don't care. Probably because I don't speak about it 😄 .

🤣 yeah, that's a complain I got from Austin (I agree tho), since he makes a bunch on live workshops / videos, etc. But like you said, it's a "breaking change".

if not we can close this PR and even #444 (feeling sad saying this 🥲 lol no JK 😂)

hahah. I'm super glad we explored & tinkered with that option. But we realized that it's not optimal for us. So... great work <3

So I am leaning towards removing ^ from package.json for now
Maybe since #541 was solved we could wait over this weekend

Let's regroup next week and see.

Thanks all!! I love having these discussions.

@rin-st
Copy link
Member

rin-st commented Sep 28, 2023

So I am leaning towards removing ^ from package.json for now

We need to remember about #541 (comment) . Not sure if removing ^ stable enough, and because of it, I'm leaning to pnpm again 😂. But it's much easier to check how locked versions work first.

Maybe since #541 was solved we could wait over this weekend to see if Vercel replies if not we can close this PR and even #444 (feeling sad saying this 🥲 lol no JK 😂)

🥲 🥲 🥲 Sad, I thought we'll go with it

Thanks all!! I love having these discussions.

🙏

@technophile-04
Copy link
Collaborator Author

Closing this for now until vercel/vercel#10614 is solved

Thanks all 🙌

@technophile-04 technophile-04 deleted the fix/deployments branch April 25, 2024 14:41
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.

bug: Vercel deployment fails
4 participants