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 nextjs example to NextJS 14 #1718

Merged
merged 14 commits into from
Sep 11, 2024
Merged

Upgrade nextjs example to NextJS 14 #1718

merged 14 commits into from
Sep 11, 2024

Conversation

smaye81
Copy link
Member

@smaye81 smaye81 commented Jul 29, 2024

This updates the example in nextjs to use Next.js v14. A few minor changes to accommodate v14:

  • Accessing dynamic data sources such as cookies is not permitted inside a cache scope, so the getMessages function we use for a fake DB had to be refactored to accept a cookie instead of doing the lookup inside: See https://nextjs.org/docs/app/api-reference/functions/unstable_cache
  • serverActions are no longer experimental and are the default so that was removed from the nextjs config file.

Signed-off-by: Steve Ayers <sayers@buf.build>
@smaye81
Copy link
Member Author

smaye81 commented Jul 29, 2024

Once connectrpc/connect-es#1159 is merged and released, we can upgrade these examples to use the new peer deps.

Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Can you check that we're using Next.js like intended?

IIRC v14 adds a path alias to tsconfig.json. We should use @-imports if that's what Next.js defaults to.

From what I remember, v14 also uses an mjs file for the config, and next-env.d.ts should be git-ignored (we should be using the .gitignore Next.js scaffolds).

nextjs/app/react-server-actions/fake-db.tsx Outdated Show resolved Hide resolved
@smaye81
Copy link
Member Author

smaye81 commented Jul 30, 2024

Regenerated a Next.js app using v14 and all suggested defaults. Hopefully this should be adhering to v14 patterns.

@timostamm
Copy link
Member

Looks great! Keeping the scaffolded files up-to-date is super helpful for users setting up a project, and it gives us confidence that we're compatible.

I don't think we should use Tailwind though 😬

I didn't realize that create-next-app saved my settings (can delete them with npx create-next-app@latest --reset-preferences) and though it wasn't a default choice.

Tailwind is actually fine, but we shouldn't have Tailwind on board and then use CSS modules instead. I guess we opted for CSS modules because it's much more simple, and we don't care about CSS in our examples.

So we should either remove Tailwind, or port the styles to use Tailwind.

smaye81 added 10 commits July 30, 2024 14:54
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
Signed-off-by: Steve Ayers <sayers@buf.build>
@smaye81 smaye81 force-pushed the sayers/nextjs_14 branch from e8aa104 to 80d5877 Compare July 30, 2024 18:54
@smaye81
Copy link
Member Author

smaye81 commented Jul 30, 2024

Removed Tailwind (and PostCSS) in favor of the previous CSS modules we've always used. Also updated the README to reflect that we opted out of Tailwind.

@timostamm
Copy link
Member

Nice, let's release the upstream update and get this merged.

@smaye81 smaye81 merged commit d156c74 into main Sep 11, 2024
6 checks passed
@smaye81 smaye81 deleted the sayers/nextjs_14 branch September 11, 2024 14:40
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.

2 participants