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 to version 14 #1719

Merged
merged 10 commits into from
Feb 21, 2024
Merged

Upgrade Next to version 14 #1719

merged 10 commits into from
Feb 21, 2024

Conversation

henrycatalinismith
Copy link
Collaborator

@henrycatalinismith henrycatalinismith commented Dec 17, 2023

This PR upgrades from Next v12 to Next v13v14. The general idea here is to take this a little bit slowly and break it up into very clearly differentiated steps. Ideally this PR will closely follow the official v13 upgrade instructions because nothing will go wrong that takes us outside that happy path.

To get this started, I'm just pushing a plain package.json & yarn.lock update because I think it'll be fun to surface any immediate build errors in public.

@henrycatalinismith
Copy link
Collaborator Author

So that very naive version bump commit gave the following error.

$ cross-env NODE_ENV=production playwright test
Running 79 tests using [4](https://github.com/zetkin/app.zetkin.org/actions/runs/7240242655/job/19723004048?pr=1719#step:7:5) workers
TypeError: Cannot read properties of undefined (reading '0')
at nextBuild (/home/runner/work/app.zetkin.org/app.zetkin.org/node_modules/next/src/cli/next-build.ts:47:3[5](https://github.com/zetkin/app.zetkin.org/actions/runs/7240242655/job/19723004048?pr=1719#step:7:6))
at globalSetup (/home/runner/work/app.zetkin.org/app.zetkin.org/integrationTesting/globalSetup.ts:21:[11](https://github.com/zetkin/app.zetkin.org/actions/runs/7240242655/job/19723004048?pr=1719#step:7:12))
Error: TypeError: Cannot read properties of undefined (reading '0')

I asked Copilot what could be causing that, and it said:

The error message "TypeError: Cannot read properties of undefined (reading '0')" typically means that you're trying to access an element of an array or object that doesn't exist. In this case, it seems like the nextBuild function is expecting an array as an argument, but it's not getting what it expects.

In Next.js v13, the nextBuild function from next/dist/cli/next-build has been removed or changed, which is likely causing this issue.

You should use the next build command directly in your CI/CD pipeline instead of importing it from next/dist/cli/next-build.

The code in the follow-up commit is more or less Copilot's suggestion except with its usual silly mistakes corrected. It seems to have progressed the build to the next error, so a successful first step.

@henrycatalinismith
Copy link
Collaborator Author

henrycatalinismith commented Dec 17, 2023

The next error's NextRouter was not mounted, which occurs like 100+ times during the unit tests. I've quickly tried replacing all the useRouter imports with next/compat/router but that's just returning null for the unit tests and so all the same tests are failing except with a different error message. Calling it a night there, good start!

@richardolsson
Copy link
Member

The one remaining issue I can see now when looking around the pages is that there have been some sort of changes to links. The MUI links now render in blue, and errors are thrown in some places because the Nextjs Link component no longer likes to have a elements as children. So I will take on that work and fix it everywhere.

In the meantime @henrycatalinismith, maybe you can start playing with merging the work from this branch with the survey work, and begin moving the public survey page over to the app router?

@henrycatalinismith
Copy link
Collaborator Author

git checkout issue-1718/next-v13-upgrade
git rebase main
git push -f origin issue-1718/next-v13-upgrade

This was referenced Jan 30, 2024
@henrycatalinismith henrycatalinismith linked an issue Feb 3, 2024 that may be closed by this pull request
@henrycatalinismith henrycatalinismith changed the title Upgrade Next to version 13 Upgrade Next to version 14 Feb 3, 2024
@henrycatalinismith
Copy link
Collaborator Author

Here's a sped-up screen recording showing a random series of interactions throughout the admin interface after running rm -rf .next node_modules; yarn; yarn devserver in this branch. Green build and nothing jumping out as obviously broken during exploratory testing, so it's over to you now @richardolsson! See what you think!

14.mov

@henrycatalinismith henrycatalinismith marked this pull request as ready for review February 3, 2024 11:33
@richardolsson
Copy link
Member

Nice work! I will look more closely into this after we release some stuff that's in the pipeline.

@richardolsson
Copy link
Member

We have recently merged the new importer (huge PR #1599). Can you try merging main into this now and make sure this still works @henrycatalinismith?

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

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

I've looked at this more closely and I think it looks fine. I'd like us to merge #1793 into this first though, so that we can get a preview live and have more people play around with it before we merge. Will request review there!

@richardolsson
Copy link
Member

@ziggabyte
Copy link
Contributor

Super nice to have the upgrade!! I have clicked around and did not experience anything strange, so it's a yes for me! :D

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

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

I have reviewed all the code, and also looked around the deployed preview, and I agree that it all looks good. Let's merge!

@richardolsson richardolsson merged commit 1138de3 into main Feb 21, 2024
5 checks passed
@richardolsson richardolsson deleted the issue-1718/next-v13-upgrade branch February 21, 2024 10:44
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.

Next.js Upgrade
3 participants