vercel / nextjs-subscription-payments

Clone, deploy, and fully customize a SaaS subscription application with Next.js.
https://subscription-payments.vercel.app/
MIT License
6.77k stars 1.38k forks source link

Server-rendered Navbar prevents static page prerendering #336

Open GabeDahl opened 6 months ago

GabeDahl commented 6 months ago

The Navbar component is server-rendered. Since it is included in the root layout, this forces every single page to be server-rendered on demand (dynamic).

Because of this, the time to first byte often exceeds 1 second.

If the maintainers are open to it, I can rework the Navbar and its auth-awareness to avoid this hiccup.

thorwebdev commented 6 months ago

Thanks @GabeDahl for digging into this and reporting!

@dalkommatt and @chriscarrollsmith I remember y'all doing this as part of the awesome Next.js 14 rework. Do you happen to have any guidance on this?

chriscarrollsmith commented 6 months ago

@thorwebdev @GabeDahl I have an open PR with a full rework of the navbar:

https://github.com/vercel/nextjs-subscription-payments/pull/313

I can't speak to the time to first byte or the rendering sequence, because I haven't done any benchmarking on it. But I think it may resolve the problem? Give it a look.

GabeDahl commented 6 months ago

@chriscarrollsmith I cloned your PR and removed all the async stuff from the home page (pricing page). So the root page.tsx is just:

export default function PricingPage() {
  return (
    <div>hello world</div>
  );
}

This should result in a static, pre-rendered page during the build process. However: image

You can see that all routes are still dynamic, and thus rendered on-demand at request time. This means that a website visitor will have to wait for server to cold-start & render the route instead of just serving the static content. (hence, a long time to first byte)

If you remove the navbar, static generation and pre-rendering work as expected. So it appears that including a server component in the root layout prevents static rendering.

GabeDahl commented 6 months ago

I ended up replacing the server-rendered component with a client component (I get the session, but that can be easily swapped for user):

'use client'
import { createClient } from '../../../utils/supabase/client';

...

export default function Header() {
  const path = usePathname();
  const [session, setSession] = useState<Session | null>();
  useEffect(() => {
    supabase.auth.getSession().then(({ data: { session } }) => {
      setSession(session);
    });

    const {
      data: { subscription }
    } = supabase.auth.onAuthStateChange((_event, session) => {
      setSession(session);
    });

    return () => subscription.unsubscribe();
  }, [path]);
  return (

...

It's not very elegant, but that's how I worked around some wonky issues with Next's root layout and direct child components (in other words, child components that are not part of any page, just the root layout)

dalkommatt commented 6 months ago

Hi @GabeDahl, I'm interested to hear how you would rework it. The whole point of a navbar is that it's present on every page. I'm really hoping partial pre-rendering goes stable in Next.js 15, as it should solve this issue automatically if the <Navbar> component is wrapped in <Suspense>. Until then the only other alternative I'm aware of would be to create a sign in/sign out client component wrapped in <Suspense> and fetch the user client side, but you may run into issues there as well because supabase/ssr's createBrowserClient is not able to recognize a server-side sign in without the page being refocused or having a hard refresh.

GabeDahl commented 6 months ago

@dalkommatt You're right. SSR's browser client doesn't recognize server-side auth state changes. That's why I hacked together that ugly useEffect that fires whenever the pathname changes or whenever supabase.auth.onAuthStateChange fires (to catch client-side auth updates).

My "fix" assumes that any server-side auth updates are accompanied with navigation/redirects. This repo already does that with the handleRequest and SignOut from /utils/auth-helpers/..., but that's a dangerous assumption to apply generally.

Partial pre-rendering is definitely the ideal solution. So I see three options:

  1. Keep the current dynamic Navbar and endure a really slow TTFB.
  2. Use my workaround which [dangerously] assumes devs will navigate following any server-side auth updates.
  3. Use the experimental partial pre-rendering

It's honestly a tough call. I'll personally opt for 2 or 3 in my projects, but that might not be ideal for a template/repo like this.

chriscarrollsmith commented 6 months ago

Interesting, @GabeDahl. I appreciate your work on this. I agree that #2 or #3 is the way to go. Thanks also for the sample code; I will have to play around with that a bit. In our work on this repo, we were trying to keep auth management fully server-side for security purposes. But may have wedded ourselves a little too rigidly to that paradigm, especially considering that full SSR support in Next.js and Supabase was (and still is) still relatively new.

Raspber commented 6 months ago

This might be very pointless and not a good solution. I do not have a navbar in my layout since I am using the index page as a landing page and I have a Dashboard route that holds a layout and in there I have my main navbar that renders if the user is logged in, if not , users are redirected to the landing page.