♻️ Full migration from app router PLUS next15/react19 #450

KemingHe · 2024-12-16T01:23:07Z

Completes #375

Part 1/2 Page Router -> App Router

All existing Playwright behavior testing passing. 5b60e5b
Added new Playwright screenshot tests to all 4 image endpoints. 74809d6
Updated unit tests to reflect the new architectural changes.

Part 2/2 -> Next15/React19 via Official Codemod

Breezed through Codemod, fully upgraded deps and types, no major/minor code changes.
Updated typing based on latest docs.
Added component update timeout to Playwright user story test to prevent eaten input. aba4b28
Updated variable names based on latest owner feedback.

Thank you @wei for your tremendous support; I look forward to replying to your review.

Edit: additional context

Screenshot all 4 GET image api endpoints with complex config per viewport type (Chrome and Mobile Chrome) in preparation for migration. Also updated TailwindCSS config from .js to .ts.

…jsx dep.

Followed nextjs official docs and best practices, backed by Playwright behavior regression test passing. Upated unit tests to reflect the new framework.

changeset-bot · 2024-12-16T01:23:11Z

🦋 Changeset detected

Latest commit: b809885

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
socialify	Minor

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

netlify · 2024-12-16T01:23:37Z

✅ Deploy Preview for github-socialify ready!

Name	Link
🔨 Latest commit	`b809885`
🔍 Latest deploy log	https://app.netlify.com/sites/github-socialify/deploys/67638ce148710e000893c702
😎 Deploy Preview	https://deploy-preview-450--github-socialify.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

wei

This is wonderful 💯! I see so many great improvements.

I left a few questions above please review and lmk your thoughts.

I'll conduct a more thorough review when I'm on my computer tomorrow. 🙌

src/hooks/useRouteResources.ts

src/components/configuration/config.tsx

app/layout.tsx

Upgraded using the official NextJS codemod. No file modded by the codemod. Updated types and deps. Fully compatible with exising behavior tests, all passed.

package.json

tsconfig.json

Regressively tested all the requested changes, and determined safe to keep them. Added server-only prod dep to explicitly limit code involving GITHUB_TOKEN to only run server-side.

KemingHe · 2024-12-17T17:52:26Z

server-only is a react (not next) marker that enforces usage of the module that imports it to run on server-side. It's widely used to explicitly protect code involving server-side secrets.

I'll let you ultimately decide whether to keep it. It's extremely light-weight w/ no versioning/upgrade needed.

wei · 2024-12-17T23:56:44Z

👍 server-only is definitely a good DX and build time improvement. https://nextjs.org/docs/app/building-your-application/rendering/composition-patterns#keeping-server-only-code-out-of-the-client-environment

app/api/graphql/route.ts

wei

Could you apply these two changes for Next15 upgrade?

KemingHe · 2024-12-18T18:05:25Z

Hi @wei , I used https://realfavicongenerator.net/ to double-confirm my favicon and manifest updates are good. I did not re-generate/format any of the files, only copy/rename.

P.S. It's a known issue that their site's favicon.ico check is bugged recently, everything else works fine, so no worries.

KemingHe · 2024-12-18T18:12:09Z

I also found this unexpected behavior where the import breaks for the wasm module as soon as I touched the public dir. The fix is to revert (only) them to relative imports.

I suspect this is related to how next caches/imports? public dir and how the behavior changed once favicon.ico and manifest.json got moved out.

wei · 2024-12-18T22:39:50Z

@KemingHe That all sounds and looks great! I was previewing the feature branch and noticed the selected font is not loaded for the preview. Everything else is working well. Could you help confirm the font is loaded correctly?

KemingHe · 2024-12-19T00:54:58Z

Hi @wei , assuming you are referring to the latest https://deploy-preview-450--github-socialify.netlify.app/ preview, here's what I found:

All image urls are working (with custom fonts).
Only the Inter/Non-inter font change is working on preview config page (page refresh doesn't fix it).

I have a few ideas on the root cause, but before we get into that, what if you try to create a branch off the current working master and deploy a preview without any changes to see if the issues are also present and consistent?

If so I argue that we are actually good to merge to master. Otherwise we can keep investigating. What do you think?

wei · 2024-12-19T01:01:29Z

@KemingHe good point!

I just checked the master branch deployment: https://master--github-socialify.netlify.app
Permalink: https://67611dae31fa580008b7c232--github-socialify.netlify.app

I can confirm the other fonts are working correctly on the preview page there but not in this pull request preview url.

KemingHe · 2024-12-19T01:12:57Z

Wait, @wei , I meant the difference between preview and production deployments might be the most immediate cause (and of course, then there's the root cause). You said you checked the master production deployment, correct? What if you create a branch off from current master and trigger a preview deployment?

Edit: typo

On top of such, I also noticed that a serif default font (not in font list) is applied to the not-working fonts config cases. Are you familiar with this (only seen on preview config page, all image APIs are unaffected and fine)?

This is not the inter font we defined in the Rootlayout as Inter is sans-sarif.

wei · 2024-12-19T01:36:36Z

I believe the permalink act the same as the pull request preview deployments.

Image APIs add fonts within the svg so it's slightly different:

socialify/common/renderSVG.tsx

Line 25 in 811ccd2

fonts: await getFonts(config.font),

Whereas the preview page loads font here:

socialify/src/components/preview/preview.tsx

Lines 129 to 140 in 811ccd2

    
           <Head> 
        
             <link 
        
               href={`https://fonts.googleapis.com/css2?family=Jost:wght@400&display=swap`} 
        
               rel="stylesheet" 
        
               key="preview-card-fonts-1" 
        
             /> 
        
             <link 
        
               href={`https://fonts.googleapis.com/css2?family=${config.font}:wght@200;400;500&display=swap`} 
        
               rel="stylesheet" 
        
               key="preview-card-fonts-2" 
        
             /> 
        
           </Head>

I noticed in master, these two font stylesheet links are added to head, but not in the pull request preview domain:

I see no changes around these lines in this PR so it might be related to how <Head> and link tags are handled.

RE: font fallback. Fallback is not used as defined in root layout as the font family was overwritten without a fallback:

wei · 2024-12-19T01:44:16Z

It is bad practice to have two <head>s in an html, the nextjs <Head> is a helper component acting a portal to move whatever is in there to the actual html's <head>.

KemingHe · 2024-12-19T01:45:46Z

I propose we (I) modify the current Playwright simpleUserStory.spect.ts to cover this case. This will require a snapshot update. What do you think?

wei · 2024-12-19T01:50:05Z

Confirmed next/head does not work with app router per vercel/next.js#53529 (comment)

I'm working on an alternative.

wei · 2024-12-19T01:51:07Z

I propose we (I) modify the current Playwright simpleUserStory.spect.ts to cover this case. This will require a snapshot update. What do you think?

Sounds good! I will make an update to fix this and will let you update the snapshot. Please stay tuned~

Replaced next <Head> with html <head> in preview to resolve font not updating issue. Updated Playwright user story regressing test and snapshot to cover this weak point. Updated select wrapper component to be more accessible and compatible with Playwright. Not sure why config panel width changed after editing component, thus had to update one UI snapshot as well. Further polished pnpm scripts to better match actual flag name, i.e. pnpm test:e2e:update-snapshots to match playwright test --update-snapshots (with an 's')

KemingHe · 2024-12-19T02:27:24Z

@wei , my latest commit is a (like you said) "bad practice" fix, as it uses the html <head> in preview.tsx. Still, please use the updated user story snapshots to confirm your better solution. Excited for the great news.

wei

@KemingHe ✅ Approved!! I'm so excited.

KemingHe added 6 commits December 15, 2024 15:31

✅ Added Playwright behavior tests to image api endpoints

74809d6

Screenshot all 4 GET image api endpoints with complex config per viewport type (Chrome and Mobile Chrome) in preparation for migration. Also updated TailwindCSS config from .js to .ts.

🎨 Strongly typed preview card and card theme wrapper, separeted exports.

96a76e8

♻️ Full (verified) api migration from page to app router.

290ebf8

🎨 (Header) Moved GitHub corner styling to global.css, removed styled-…

5970ad6

…jsx dep.

♻️ Full front+backend migration from page router to app router.

5b60e5b

Followed nextjs official docs and best practices, backed by Playwright behavior regression test passing. Upated unit tests to reflect the new framework.

📝 Added changeset reflecting page to app router migration.

496030c

wei reviewed Dec 16, 2024

View reviewed changes

src/hooks/useRouteResources.ts Outdated Show resolved Hide resolved

src/components/configuration/config.tsx Show resolved Hide resolved

app/layout.tsx Outdated Show resolved Hide resolved

⬆️ Upgraded successfully to Next15/React19 with minor fixes.

b29feaf

Upgraded using the official NextJS codemod. No file modded by the codemod. Updated types and deps. Fully compatible with exising behavior tests, all passed.

KemingHe changed the title ~~♻️ Full front+backend migration from page router to app router~~ ♻️ Full migration from app router PLUS next15/react19 Dec 16, 2024

KemingHe marked this pull request as draft December 16, 2024 02:17

✅ Added component update timeout to prevent flaky tests.

aba4b28

KemingHe marked this pull request as ready for review December 16, 2024 02:31

wei reviewed Dec 17, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

wei reviewed Dec 17, 2024

View reviewed changes

tsconfig.json Outdated Show resolved Hide resolved

wei added 2 commits December 17, 2024 00:23

Merge branch 'master' into upgrade-pagarouter2approuter-keminghe

29b3660

Merge branch 'master' into upgrade-pagarouter2approuter-keminghe

d00a7b9

wei force-pushed the upgrade-pagarouter2approuter-keminghe branch from b0e8cca to d00a7b9 Compare December 17, 2024 06:49

KemingHe added 2 commits December 17, 2024 12:31

🎨 Polished code based on comments and added server-only dep

c2660ea

Regressively tested all the requested changes, and determined safe to keep them. Added server-only prod dep to explicitly limit code involving GITHUB_TOKEN to only run server-side.

⬆️ Upgraded depedencies to latest patch/minor verisons w/out breakage.

f9ac9f3

wei reviewed Dec 17, 2024

View reviewed changes

app/api/graphql/route.ts Show resolved Hide resolved

wei requested changes Dec 18, 2024

View reviewed changes

📝 Updated favicon, web app icons, and manifest per nextjs standard.

4f95a6e

wei force-pushed the upgrade-pagarouter2approuter-keminghe branch from 797d168 to 4030c8e Compare December 19, 2024 03:01

🐛 Fix preview font, update linting and snapshots

b809885

wei force-pushed the upgrade-pagarouter2approuter-keminghe branch from 4030c8e to b809885 Compare December 19, 2024 03:02

wei enabled auto-merge (squash) December 19, 2024 03:03

wei approved these changes Dec 19, 2024

View reviewed changes

wei merged commit d73c1be into wei:master Dec 19, 2024
7 checks passed

wei mentioned this pull request Dec 19, 2024

Upgrade to Next.js 15 #373

Closed

KemingHe deleted the upgrade-pagarouter2approuter-keminghe branch December 19, 2024 04:32

wei mentioned this pull request Dec 19, 2024

Unexpected Caching after Next.js 15 Upgrade #462

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

♻️ Full migration from app router PLUS next15/react19 #450

♻️ Full migration from app router PLUS next15/react19 #450

KemingHe commented Dec 16, 2024 •

edited

Loading

changeset-bot bot commented Dec 16, 2024 •

edited

Loading

netlify bot commented Dec 16, 2024 •

edited

Loading

wei left a comment

KemingHe commented Dec 17, 2024

wei commented Dec 17, 2024

wei left a comment

KemingHe commented Dec 18, 2024

KemingHe commented Dec 18, 2024

wei commented Dec 18, 2024

KemingHe commented Dec 19, 2024

wei commented Dec 19, 2024 •

edited

Loading

KemingHe commented Dec 19, 2024 •

edited

Loading

wei commented Dec 19, 2024 •

edited

Loading

wei commented Dec 19, 2024 •

edited

Loading

KemingHe commented Dec 19, 2024

wei commented Dec 19, 2024

wei commented Dec 19, 2024

KemingHe commented Dec 19, 2024

wei left a comment

♻️ Full migration from app router PLUS next15/react19 #450

♻️ Full migration from app router PLUS next15/react19 #450

Conversation

KemingHe commented Dec 16, 2024 • edited Loading

Part 1/2 Page Router -> App Router

Part 2/2 -> Next15/React19 via Official Codemod

changeset-bot bot commented Dec 16, 2024 • edited Loading

🦋 Changeset detected

netlify bot commented Dec 16, 2024 • edited Loading

✅ Deploy Preview for github-socialify ready!

wei left a comment

Choose a reason for hiding this comment

KemingHe commented Dec 17, 2024

wei commented Dec 17, 2024

wei left a comment

Choose a reason for hiding this comment

KemingHe commented Dec 18, 2024

KemingHe commented Dec 18, 2024

wei commented Dec 18, 2024

KemingHe commented Dec 19, 2024

wei commented Dec 19, 2024 • edited Loading

KemingHe commented Dec 19, 2024 • edited Loading

wei commented Dec 19, 2024 • edited Loading

wei commented Dec 19, 2024 • edited Loading

KemingHe commented Dec 19, 2024

wei commented Dec 19, 2024

wei commented Dec 19, 2024

KemingHe commented Dec 19, 2024

wei left a comment

Choose a reason for hiding this comment

KemingHe commented Dec 16, 2024 •

edited

Loading

changeset-bot bot commented Dec 16, 2024 •

edited

Loading

netlify bot commented Dec 16, 2024 •

edited

Loading

wei commented Dec 19, 2024 •

edited

Loading

KemingHe commented Dec 19, 2024 •

edited

Loading

wei commented Dec 19, 2024 •

edited

Loading

wei commented Dec 19, 2024 •

edited

Loading