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

Rearchitecture #10

Merged
merged 99 commits into from
Aug 13, 2023
Merged

Rearchitecture #10

merged 99 commits into from
Aug 13, 2023

Conversation

edwardshturman
Copy link
Member

@edwardshturman edwardshturman commented Jul 10, 2023

Rearchitecting Cue

What

A comprehensive internal rearchitecting of Cue, including migrating to the Next.js App Router.

Why

Right so here's the deal:

Cue was written during a hackathon. As such, in a time-sensitive environment, certainly one in which I knew muuuuch less about React and Next, corners were cut. Cue code isn't that composable: for example, instead of a Card component with child props passed in, there's a ProfileCard, a ResultsCard, etc.

Unfortunately, given how unergonomic the codebase has been up to this PR, an App Router migration is far from simple. A full-fledged rearchitecture of Cue is necessary, both for compatibility's sake, as well as to make improving Cue easier down the line.

Why App Router?

While the Next.js App Router has faced some controversy in its early days of stability, there are a few reasons I want to embrace it:

  1. 🚀 It's the future of Next.js, and React: the App Router provides first-class support for React Server Components. This is the direction of React's mental model, and while it requires a shift in thinking, I believe it's worth undertaking.
  2. ⚡ It's like, really fast: shipping less JavaScript to the client (by virtue of RSCs, which are rendered on the server as the name suggests) is exponentially impactful as the project grows.
  3. ✨ Shiny object syndrome: as a college student and lifelong learner, I've gotta embrace & explore new tech. :) I'm very fortunate to work on a project like Cue, which has a lot of room for iteration velocity — moving fast, making mistakes, and learning from them.

The plan

I haven't a clue how long it'll take, but ideally by mid-August this'll be out the gate. What sucks is this is obviously not relevant to the actual students using our app, and on the surface it will seem like a lack of new features. But I can say with a fair bit of certainty this'll be good for the project. :)

Below is a detailed checklist of what's what, and I'll post regular progress updates as comments.
Thank you for following along! 🤍

Progress

Migrating routes to the App Router

  • Root layout
  • Sign in page
  • Auth.js APIs
  • Cue generation API
  • Card invites
  • Profile page
  • Cue generation page
  • Invite code page
  • Redeem invite page

Authentication

  • Auth policies #11 & logic
  • Remove user API routes
  • Suite of helper functions for user management
  • Upgrade to (experimental) NextAuth v5
  • Migrate users from MongoDB to Vercel KV

Invite system 2.0 prep

  • Extract invite validation logic
  • Migrate invites from MongoDB to Vercel KV
  • Migrate invite-schema dependents (other than debug functions) over to Vercel KV

Usage tracking & rate limiting (#16)

  • Add generations object array
  • Count tokens used in a generation
  • Push to a user's generations
  • Implement rate limits
  • Handle user hitting rate limit gracefully

QoL

  • Fix sidebar layout shift
  • Swath of semantics clarifications
  • Revert to npm
  • Hide nonfunctional "View saved cues" button
  • Fix broken styles from Styled JSX not working in the new Root Layout
  • Remove unused icons

Notable differences:
• Metadata API
• Extracted Auth.js `SessionProvider` to new `AuthWrapper` client component
• Extracted Sidebar conditional rendering to component-level

Known issues:
• It's bugging out about the styles
• There was something with the `children` not being a valid React child or something like that
Notable differences:
• Extracted sign in button to its own client component
• Removed `next/head`, now taken care of by Metadata API
• Returning a redirect object in `getServerSideProps` → `next/navigation` redirect

Known issues:
• Styles not working (todo switch from styled JSX)
• Sidebar rendering "Loading..."
@vercel
Copy link

vercel bot commented Jul 10, 2023

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
cue ⬜️ Ignored (Inspect) Visit Preview Aug 12, 2023 11:59pm

@edwardshturman edwardshturman self-assigned this Jul 10, 2023
@edwardshturman
Copy link
Member Author

edwardshturman commented Jul 10, 2023

Noting perf issues in Dev Mode, Next.js team working on it

@edwardshturman
Copy link
Member Author

edwardshturman commented Jul 21, 2023

I pretty much moved the below comment to the PR description lol, but this was what I posted when I decided the PR would be a full-on rearchitecture instead of just an App Router migration:

Right so here's the deal:

Cue was written during a hackathon. As such, in a time-sensitive environment, certainly one in which I knew muuuuch less about React and Next, corners will be cut. And...they were. Cue code isn't that composable: for example, instead of a Card component with child props passed in, there's a ProfileCard, a ResultsCard, etc.

What does this have to do with App Router? Well, other than the incessant itch to fix things here and there in a targeted PR — you know how these things go — an App Router migration is proving to be far from ergonomic in the current situation. From invite logic to duplicate components, this is something best to tackle sooner than later.

What's the plan? I haven't a clue how long it'll take, but ideally by mid-August this'll be out the gate. To be clear, "this" means a complete internal rearchitecting of Cue. What sucks is this is obviously not relevant to end-users, and on the surface it only seems like a lack of new features. But I can guarantee it'll make it easier for us — and you! :) — to improve Cue.

Will continue work on this branch, pardon the scope creep. Thank you for following along! 🤍

@edwardshturman edwardshturman changed the title Migrate to Next.js App Router Rearchitecture Jul 21, 2023
@edwardshturman
Copy link
Member Author

Today's progress report: pretty happy with hitting push on moving /profile to App Router, even though this code is unstable at the moment. There are two major efforts I want to focus on for tomorrow:

  1. Moving the rest of the /profile getServerSideProps code, which handled user creation and invite logic, over to the new check-auth set of utility functions.
  2. Migrating back to fetching user session and data server-side, rather than exposing an API and calling it, resulting in an unnecessary round trip, plus security implications. At the moment there are no plans to allow external user creation anyway; for extensions, we can just rewrite the relevant user fetching logic.

@edwardshturman edwardshturman removed a link to an issue Aug 11, 2023
@edwardshturman
Copy link
Member Author

Almost done, so close.... Would like to include token usage in a user's generations, at least until it can be accounted for elsewhere and play into #21.

@edwardshturman
Copy link
Member Author

It's hard to believe this is ready... 🥲

I do feel like I learned a lot, not just new technical knowledge, but also good practice, like maybe to check against dependencies before committing to something so big lol

Feels good. Will have this merged later today ✌️

@edwardshturman
Copy link
Member Author

Just realized almost 50% of Cue code history was dedicated to this lol

@edwardshturman edwardshturman merged commit 2528bd1 into dev Aug 13, 2023
2 checks passed
edwardshturman added a commit that referenced this pull request Aug 13, 2023
@edwardshturman edwardshturman deleted the meta/next-app-router-migration branch August 13, 2023 00:35
edwardshturman added a commit that referenced this pull request Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature 🌟 New feature or major change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Usage tracking & rate limiting Auth policies
1 participant