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

Upgrade Next.js to v13 #5402

Merged
merged 33 commits into from
Jan 15, 2025
Merged

Upgrade Next.js to v13 #5402

merged 33 commits into from
Jan 15, 2025

Conversation

nabramow
Copy link
Member

@nabramow nabramow commented Dec 28, 2024

Closes #5154

Next v13 upgrade. I will do the v14 upgrade and conversion to the appRouter in a separate PR.

Migration Guide:
https://nextjs.org/docs/pages/building-your-application/upgrading/version-13

I needed to upgrade these packages as well for this:

  • i18next
  • next-i18next
  • react-i18next
  • @sentry/nextjs
  • @sentry/react
  • sentry-testkit

This means you'll want to pay attention to translations and sentry while testing.

Web frontend checklist

  • [ x ] Formatted my code with yarn format
  • [ x ] There are no warnings from yarn lint --fix
  • There are no console warnings when running the app
  • [ x ] All tests pass
  • [ x ] Clicked around my changes running locally and it works
  • [ x ] Checked Desktop, Mobile and Tablet screen sizes

Copy link

vercel bot commented Dec 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
couchers ✅ Ready (Inspect) Visit Preview Jan 15, 2025 4:53am

@nabramow nabramow marked this pull request as ready for review January 4, 2025 06:49
@nabramow nabramow marked this pull request as draft January 4, 2025 06:55
Copy link
Contributor

@jesseallhands jesseallhands left a comment

Choose a reason for hiding this comment

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

I couldn't find any issues during testing. The only issue I encountered I think is unrelated to this PR:

Sending a request with a duration of over 1 year clears both dates AND clears the request message you typed (the entire text is deleted). This would really suck if you typed a long request:
image

@nabramow
Copy link
Member Author

nabramow commented Jan 4, 2025

I couldn't find any issues during testing. The only issue I encountered I think is unrelated to this PR:

Sending a request with a duration of over 1 year clears both dates AND clears the request message you typed (the entire text is deleted). This would really suck if you typed a long request: image

Oh interesting but yeah that's unrelated let me throw that in another ticket here: #5432

@nabramow nabramow changed the title Upgrade Next.js to v13 - Part I Upgrade Next.js to v13 Jan 12, 2025
aapeliv
aapeliv previously approved these changes Jan 14, 2025
Copy link
Member

@aapeliv aapeliv left a comment

Choose a reason for hiding this comment

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

Looks good to me codewise! Is the empty <a> thing something you just picked up here or is it raising errors now in the new version of Next? Either way great you cleaned it up!

@nabramow
Copy link
Member Author

nabramow commented Jan 15, 2025

Looks good to me codewise! Is the empty thing something you just picked up here or is it raising errors now in the new version of Next? Either way great you cleaned it up!

Ah it was part of the upgrade to remove the <a> tags, see here (second to last point):
https://nextjs.org/docs/pages/building-your-application/upgrading/version-13#v13-summary

I imagine something changed under the hood there so that we no longer need it.

@nabramow
Copy link
Member Author

FYI Merging as aapeli approved but it went away when I fixed the merge conflicts.

@nabramow nabramow merged commit d19054c into develop Jan 15, 2025
3 checks passed
@nabramow nabramow deleted the na/upgrade-next branch January 15, 2025 06:02
@darrenvong
Copy link
Member

darrenvong commented Jan 16, 2025

v14 upgrade and conversion to the appRouter in a separate PR

To my best knowledge, I don't think they have plans to deprecate/remove support for the pages router, so an entire app conversion might be unnecessary? I'd probably start trying out the app routers for new routes only, although that probably need an entire app architecture re-think on how to use/not use React Query together with the app router. As things are, it feels like they conflict with each other 😅

Also soz for being a bit useless at reviewing PRs lately, but it seems like most of the changes are around link updates and tweaking the translation to work? Great job for powering through it! 🚀

@nabramow
Copy link
Member Author

v14 upgrade and conversion to the appRouter in a separate PR

To my best knowledge, I don't think they have plans to deprecate/remove support for the pages router, so an entire app conversion might be unnecessary? I'd probably start trying out the app routers for new routes only, although that probably need an entire app architecture re-think on how to use/not use React Query together with the app router. As things are, it feels like they conflict with each other 😅

Also soz for being a bit useless at reviewing PRs lately, but it seems like most of the changes are around link updates and tweaking the translation to work? Great job for powering through it! 🚀

No worries, I really appreciate you popping in at key moments! This was a good call out. What I liked about app router was it meant we could get rid of the next-18n-next package and there were some hackarounds we have that we'd be able to remove with it. But after thinking about it, there's so much important feature work to be done (like the map re-design that's blocking tons of stuff), so might be better to do less for now and return to it later if I want.

The actual conversion to the app router was pretty straight-forward tbh! It was the i18 stuff that got really complicated, and made it hard to do incrementally.

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.

Upgrade next to v13
4 participants