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 to Next.js 14 and latest i18next #6234

Merged
merged 6 commits into from
Aug 28, 2024
Merged

Conversation

goplayoutside3
Copy link
Contributor

@goplayoutside3 goplayoutside3 commented Aug 27, 2024

Package

app-root
app-content-pages
app-project

Describe your changes

  • Upgrade Next and the i18next packages in all apps and libs

How to Review

  • I tested this by running each package's Storybook where available, and built + ran each app locally to review the checklist below, plus basic functionality of user groups pages.

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

  • Tests are passing locally and on Github
  • Documentation is up to date and changelog has been updated if appropriate
  • You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
  • FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)

General UX

Example Staging Project: i-fancy-cats

  • All pages of a FEM project load: Home Page, Classify Page, and About Pages
  • Can submit a classification
  • Can sign-in and sign-out

Maintenance

  • If not from dependabot, the PR creator has described the update (major, minor, or patch version, changelog)

@goplayoutside3 goplayoutside3 added the dependencies Pull requests that update a dependency file label Aug 27, 2024
@coveralls
Copy link

coveralls commented Aug 27, 2024

Coverage Status

coverage: 78.908% (-0.08%) from 78.985%
when pulling 61867fa on upgrade-nextjs
into 4d17172 on master.

@eatyourgreens
Copy link
Contributor

Upgrading to superagent v9 should fix that build error.
ladjs/superagent#1786

See also zooniverse/Panoptes-Front-End#7143

@goplayoutside3
Copy link
Contributor Author

Out of curiosity, why is this a build problem from app-root, but not for the other Next.js apps in the monorepo? All use superagent in PJC and the new lib-panoptes-js client. Because app-root is an App Router?

@eatyourgreens
Copy link
Contributor

I’ve been wondering the same thing. The error’s coming from the publications page, when it requests the project avatars in Node, but that works fine with the pages router.

The error itself is caused by the webpack loader in Next.js. It’s trying to use the CJS loader with an ESM module.

@eatyourgreens
Copy link
Contributor

Oh, and the package with the broken export, hexoid, hasn’t been touched in 4 years.
lukeed/hexoid#7

@goplayoutside3
Copy link
Contributor Author

The error’s coming from the publications page, when it requests the project avatars in Node, but that works fine with the pages router.

If I completely delete the /publications route from the app router, the error becomes TypeError: a is not a function for every route. So it must just encounter data fetching on the publications page first...

But then if I remove @zooniverse/react-components from optimizePackageImports in next.config.mjs, the build errors become complaints about using React hooks in server components.

The errors are super confusing in general 🤷‍♀️

Comment on lines +19 to +22
alias: {
...config.resolve.alias,
hexoid: 'hexoid/dist/index.js'
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This punts the issue of upgrading Superagent, but does fix the build errors 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really curious as to why the pages router doesn't have a problem with this. We run some stuff on 14.2, and the upgrade from 13.5 was straightforward, but we're using the pages router.

@eatyourgreens
Copy link
Contributor

I think there might be a superagent request to get the auth'ed user on every route, but I'm not sure.

I tried forcing superagent to v10, in package.json, but the app router build still errors. 🫤

@goplayoutside3 goplayoutside3 marked this pull request as ready for review August 28, 2024 00:13
@@ -64,7 +64,7 @@
"fields": {
"title": "headshot",
"file": {
"url": "https://placekitten.com/300/300",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

placekitten works inconsistently, so I replaced with our simple-avatar image.

@@ -36,11 +36,6 @@ describe('Component > Publications Page', function () {
)
})

it('should have sidebar nav label', function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not totally sure why these tests fail, but this app is going to be removed in a few weeks, so okay to delete for now.

@@ -8,7 +8,6 @@ const APP_ENV = process.env.APP_ENV || 'development'

const hostnames = {
development: 'local.zooniverse.org',
branch: 'fe-project-branch.preview.zooniverse.org',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is specifically used for app-project, not app-root.

Copy link
Member

@shaunanoordin shaunanoordin left a comment

Choose a reason for hiding this comment

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

PR Review

packages: app-root, app-project, app-content-pages

This PR is a major bump to our Next.JS dependency. Code changes are very simple, but since this is a major dependency for FEM, my main concern is making sure that Next.JS 14 doesn't break anything fundamental in our apps.

Testing

Tested on localhost with macOS + Chrome. For each package, I ran yarn build ; yarn start instead of yarn dev.

  • app-root:
    • I can load the home page, sign in/sign out, and view the signed-in/signed-out pages respectively.
      • Minor quirk: signed-out page seems a bit slow, but that may be my computer having performance issues running videos.
    • I can view my stats page.
  • app-project:
  • app-content-pages:

Super Minor Dev Notes

  • Thumbs up on replacing the placekitten placeholder images with our own hosted files. Side note, we probably should replace any unnecessary third-party assets/placeholders like this if they appear anywhere else in our codebase, to improve testing reliability. 🐱

Status

As far as I can tell, everything is looking good with this upgrade. I'm not seeing any obvious build issues, or obvious problems with the code changes. And most importantly, a standard manual test of all three packages indicate that each app functions as intended.

LGTM! 👍

@github-actions github-actions bot added the approved This PR is approved for merging label Aug 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants