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

Rewrite to Next.js from CRA (#108) #109

Merged
merged 8 commits into from
Jan 12, 2024
Merged

Rewrite to Next.js from CRA (#108) #109

merged 8 commits into from
Jan 12, 2024

Conversation

jlevine18
Copy link
Contributor

  • Rewrite to Next.js

  • Fix file capitalization

* Rewrite to Next.js

* Fix file capitalization
@jlevine18 jlevine18 requested review from a team as code owners January 12, 2024 13:22
Copy link

cloudflare-pages bot commented Jan 12, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 309551d
Status: ✅  Deploy successful!
Preview URL: https://2e3f0096.acmuiuc.pages.dev
Branch Preview URL: https://cra-to-nextjs.acmuiuc.pages.dev

View logs

@jlevine18
Copy link
Contributor Author

jlevine18 commented Jan 12, 2024

TYSM @WhiteHoodHacker for doing this- looks great. I'm not entirely sure why our CF build fails- @devksingh4, do you have any ideas?

Oh, I'm very dumb and didn't realize that there needed to be changes to actions. This is why you don't do things at 7AM so that you can feel productive when you are woken up by yet another ceiling leak in your Smile apartment.

@jlevine18
Copy link
Contributor Author

Copying from Minh's original comment- will work on this later today if I have time

Closes #93

Summary of Changes

Here is my own Cloudflare Pages deployment to allow for testing: https://acm-uiuc-website.pages.dev/

Performance Improvements

Next.js with static exports allows the site to load much faster than CRA since the HTML has been pre-rendered. The main website now also functions without JavaScript needing to be enabled.

Routing Changes

Since CRA previously used hash routing to support SPAs, routes were previously explicitly defined in src/App.tsx. This has been replaced with the Next.js App Router, which creates routes based on the file structure in src/app.

I created two route groups, src/app/(main) and src/app/(membership), which are used to distinguish the layouts of the main website and event/membership related routes. Pages which were previously defined in src/pages have been moved to these respective folders.

  • / -> /
  • /#/about -> /about
  • /#/event -> /event
  • /#/event-paid -> /event-paid
  • /#/membership -> /membership
  • /#/paid -> /paid

To support the transition from hash routes to static routes, I have added a script to the / route which can detect hash-based routes and perform a client-side redirect (see here).

Additionally, the previous /#/event and /#/event-paid routes could accept an event ID parameter. The way the parameter is passed has been changed in order to support static routes.

  • /#/event/$EVENT_ID -> /event?id=$EVENT_ID
  • /#/event-paid/$EVENT_ID -> /event-paid?id=$EVENT_ID

Examples:

  • /#/event/fa23_barcrawl -> /event?id=fa23_barcrawl
  • /#/event-paid/fa23_barcrawl -> /event-paid?id=fa23_barcrawl

Package Changes

Several packages were required to be updated in order to work with the latest version of Next.js. Most notably, @nextui-org/react had a major version change and required framer-motion and tailwindcss to be installed in the main project. The introduction of Tailwind resulted in some styling changes which I mention below.

Storybook was also updated from major 6 to major 7, which didn't impact the repo much, at least when I using npm during personal development. However, when I was preparing this PR to work with yarn 1.22.21, I started running into build issues related to yarn, ESM, and a dependency of the latest version of Storybook. I found an official workaround here, but it requires either yarn to be updated to version 3 or to manually resolve the dependency to a working version. I have opted for the manual resolution (see here). However, seeing how little Storybook is used in the current development process, I would consider removing it entirely.

Beyond that, I added serve as a dev dependency to facilitate testing production builds and organized some dependencies that should have been installed as dev dependencies.

Styling Changes

Tailwind preflight was affecting some of the original styles on the website - so I rewrote all the previous CSS-in-JS into Tailwind classes and (very few) CSS modules. Along the way, I fixed many styling issues related to smaller screen breakpoints and also restyled the committees section.

As a result, styled-components has been removed. src/styles/global.css and tailwind.config.js are used to replace the previous theme files. Notably, CSS variables are now used to define color palettes which are used throughout the site.

Fonts are also now served first-party via Next.js font optimization rather than Google Fonts.

Next Steps

  • Update yarn.lock file and cache. I have decided NOT to change the yarn.lock or the recently configured zero-install yarn cache because I do not want this PR to get insta-closed lmao.
  • Update GitHub Action for deployment. Specifically, Node version 18 is required and the output production directory has changed from build to out. I did not change the workflow file in this commit because GitHub probably wouldn't let me, so I recommend the following patch:
--- a/.github/workflows/main.yml
+++ b/.github/workflows/main.yml
@@ -22,10 +22,10 @@ jobs:
     steps:
       # Checks-out your repository under $GITHUB_WORKSPACE, so your job can access it
       - uses: actions/checkout@v3
-      - name: Use Node.js 16.x
+      - name: Use Node.js 18
         uses: actions/setup-node@v3
         with:
-          node-version: 16.x
+          node-version: 18
           cache: 'yarn'
       - run: yarn install --frozen-lockfile
       - run: yarn run build --if-present
@@ -35,7 +35,7 @@ jobs:
         shell: bash
       - run : git config user.email [email protected]
         shell: bash
-      - run : git --work-tree build add --all
+      - run : git --work-tree out add --all
         shell: bash
       - run : git commit -m "Automatic Deploy action run by github-actions"
         shell: bash
  • Similarly, update Cloudflare Pages deployment for dev builds.
  • If any backend services reference routes that were changed, particularly routes with event ID parameters, those should be updated, too.

Copy link
Contributor

@devksingh4 devksingh4 left a comment

Choose a reason for hiding this comment

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

As discussed on discord:
Use dynamic routing to support /event/[id] paradigm instead of /event?id=[id] URLs.
Setup next.js edge worker support so we can host on Cloudflare Pages while still using dynamic features (this also enables us to one-click compression and other nice comforts later down the line).

@devksingh4
Copy link
Contributor

Oh, I'm very dumb and didn't realize that there needed to be changes to actions. This is why you don't do things at 7AM so that you can feel productive when you are woken up by yet another ceiling leak in your Smile apartment.

The reason for this was actually that cf expected the output to be in built not out. Fixed next.config.js to output to built.

Copy link

gitguardian bot commented Jan 12, 2024

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

Our GitHub checks need improvements? Share your feedbacks!

@devksingh4 devksingh4 merged commit fa8e85b into main Jan 12, 2024
2 checks passed
@devksingh4 devksingh4 deleted the cra-to-nextjs branch January 12, 2024 20:13
@WhiteHoodHacker
Copy link
Member

WhiteHoodHacker commented Jan 12, 2024

Oh, I'm very dumb and didn't realize that there needed to be changes to actions. This is why you don't do things at 7AM so that you can feel productive when you are woken up by yet another ceiling leak in your Smile apartment.

The reason for this was actually that cf expected the output to be in built not out. Fixed next.config.js to output to built.

That fix does not actually work. This is for dev output cache (.next) folder. You can no longer change the output directory from out.

@devksingh4
Copy link
Contributor

Oh, I'm very dumb and didn't realize that there needed to be changes to actions. This is why you don't do things at 7AM so that you can feel productive when you are woken up by yet another ceiling leak in your Smile apartment.

The reason for this was actually that cf expected the output to be in built not out. Fixed next.config.js to output to built.

That fix does not actually work. This is for dev output cache (.next) folder. You can no longer change the output directory from out.

That fix is currently working in prod (www.acm.illinois.edu).

@WhiteHoodHacker
Copy link
Member

Ok, serve script in package.json needs to be updated to serve build too.

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.

None yet

3 participants