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

Convert to app router #39

Merged
merged 19 commits into from
Dec 23, 2023

Conversation

allenjd3
Copy link
Contributor

This updates the laravel/breeze-next starter to using the new App Router routing syntax. I am not a nextjs expert so it is very possible that I'm missing better ways to do this.

This also bumps the nextjs version to nextjs 14

Benefits to changing- App Router is the "new" way of doing routing in nextjs applications so this modernizes this starter kit.

Technically, the pages style routing is still valid so we could offer both types in separate branches or just convert to the new direction.

@vipertecpro
Copy link

Wow is this really happening ?? that's really awsomm. This is the best, thx alot. We do need app router and combo with laravel is the best.
I hope all the other guiz (like tailwind flowbite) can also start building it up.

@taylorotwell
Copy link
Member

Seems to work pretty well. A couple of questions:

  • The Nunito font doesn't seem to be rendering.
  • Can you link me to some docs explaining what the "@" sign does when naming directories, for example in "@Header".

Please mark as ready for review when answered.

@taylorotwell taylorotwell marked this pull request as draft December 15, 2023 14:47
@allenjd3
Copy link
Contributor Author

So here are the docs on the "@" sign- They are called parallel routes and I'm not actually sure if this is the best spot for them. - https://nextjs.org/docs/app/building-your-application/routing/parallel-routes

When I imagine the 'dashboard' route I think of the user doing something like having a structure similar to this where posts would be nested in the dashboard route. Something like /dashboard/posts

dashboard
  @header
    page.js
  page.js
  layout.js
  posts
    layout.js
    page.js 

In this case that @header would apply to all nested routes so it would be impossible to override the "Dashboard" in the posts route. Instead you could do something like this-

dashboard
  (main)
    @header
      page.js
    page.js
    layout.js
  posts
    @header
      page.js
    layout.js
    page.js 

that would allow you to update the header section on each nested route like dashboard/posts in this instance. The other option is to just ditch the parallel routes and colocate a Dashboard component in the dashboard directory similar to the way I added the logo in the auth routes. Which would you prefer? (PS, I'm dropping the layout in the @header and just inlining that code in the @header > page.js regardless since it seems unnecessary now that I'm looking at it again

@allenjd3 allenjd3 marked this pull request as ready for review December 16, 2023 12:40
@taylorotwell
Copy link
Member

If I was building an app that had "posts", I'm not sure I would nest it under dashboard. I would imagine having /posts.

@allenjd3
Copy link
Contributor Author

In that case it might be fine the way it is

@taylorotwell
Copy link
Member

@allenjd3 it seems strange to me that @Header would be tied to the dashboard directory though? You would likely want to use that basic header on lots of pages?

@allenjd3
Copy link
Contributor Author

allenjd3 commented Dec 16, 2023

Yeah what's odd about parallel routes is that they don't support nesting- Here is a discussion that talks about this limitation: vercel/next.js#48927 - In this case it would be nice to have the slot in the layout one level above the dashboard and then ideally you could override it in nested routes. But it currently requires the @header to be in the same directory and it would be inherited by all children. So in the case we were discussing earlier if you had-

(app)
  page.js
  layout.js
  @header
    page.js
  dashboard
    etc...
  posts
    etc...

Then /dashboard and /posts would both have the exact same @header-

Perhaps in this case a wrapping component still makes more sense?
Edit: @header might not be a great name either since it really is just the section that names the page "Dashboard" or "Posts". The navigation area is already one level up in the wrapping layout

@allenjd3
Copy link
Contributor Author

I went ahead and simplified it by recreating the DashboardLayout component. Feel free to walk that commit back if you want to keep the other way of doing it!

@allenjd3
Copy link
Contributor Author

allenjd3 commented Dec 18, 2023

Alright I finally figured out how to do the @header the proper way... Sorry for all the back and forth 😅

Looks like you can do this-

(app)
  @header
    dashboard
      page.js
  dashboard
    page.js
    layout.js
  layout.js

where the root route has the reference to the header and the header names routes that are "parallel" to the ones that you create using the normal routing. Makes sense now that I get it.

@aligajani
Copy link

@allenjd3 Nice work. There is one thing we need to look into. How to prevent a flash on login page when say someone is visiting it from an authenticated area? Not sure how you would approach this with Next 14 and this Breeze starter kit. Any tips?

@allenjd3
Copy link
Contributor Author

I put in a possible solution to the flicker- I'm researching how to do this using the new Suspense api. I'll update this if I can figure it out!

@aligajani
Copy link

aligajani commented Dec 23, 2023 via email

@taylorotwell taylorotwell merged commit 52128f8 into laravel:master Dec 23, 2023
@bjaspire
Copy link

I got Error: Hydration failed because the initial UI does not match what was rendered on the server. on login page.

@allenjd3
Copy link
Contributor Author

If you have something like lastpass enabled on the page it will inject divs that don't play well with nextjs in dev mode

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.

5 participants