vercel / commerce

Next.js Commerce
https://demo.vercel.store
MIT License
11.47k stars 4.24k forks source link

Performance levers around router.push and Suspense #1334

Open mhart opened 6 months ago

mhart commented 6 months ago

Hey there – I've been playing around with this a bit, and I'm seeing a bunch of slowness (from Melbourne, Australia) on the product pages on demo.vercel.store – especially when changing images, or selecting sizes/colours. Looking under the hood, I can see it's doing full page re-renders (albeit in RSC) for each of these – which consist of at least one waterfall where the related product fetch depends on the previous fetch for the product. And the rendering of, say, the selected colour/size won't complete until the recommendations carousel has completed rendering. So each image change or size/colour selection is depending on at least two serial fetches to the Shopify API, even though nothing is actually changing and the Gallery and VariantSelector components are both client components.

Example image change on demo.vercel.store:

Screenshot 2024-05-07 at 8 48 21 AM

So I had a few questions around this.

  1. Why is it doing full server-side rerenders for each of these selections? Can't the router just do this logic client side? It feels like it should have all the props to do so? Is there an option on the router.push or Link to have this just done client-side?
  2. Assuming we do keep it server-side, it seems that putting the RelatedProducts component in a Suspense should fix this – and I tried it, it sort of does, to a degree. It means that product page loads have better TTFB as they'll render up until the RelatedProducts Suspense, and then wait for that to complete before continuing. But image changes and size/colour changes continue to require the RelatedProducts fetch to complete before rendering correctly, so it's still just as slow for these.
  3. I tried different ways to get around the latter issue, but I couldn't find any good options. For example – I can actually speed it up quite a bit by supplying a key to a Suspense that wraps RelatedProducts that's based on the searchParams. This means that the Suspense will rerender each time the search params change on that page, which means the client components are interactive quicker. The downside is that you then have a flash of content for the RelatedProducts carousel as it falls back each time.

You can see an example here: https://nextjs-commerce-3ii.pages.dev/ (this is on Cloudflare Pages, but you get the idea)

So the TLDR is: is there a way these product pages can be sped up a bit so that image/size/colour changes don't actually depend on the product recommendations fetch to complete (even if it's cached)?

leerob commented 6 months ago

The answer is here! https://github.com/vercel/commerce/pull/1327

mhart commented 6 months ago

Ah awesome, that does look much nicer.

So just a Next.js question then – why does the original (the current one) cause a full page render? ie, what signal is happening that's causing Next.js to say "aha, search params have changed, I need to rerender the full page here" rather than "aha, search params have changed, I can just update this component client-side"?

mhart commented 6 months ago

Have had a play – I don't think that really fixes the (full) problem – it's still waiting for the full page render before updating the search params.

So the UI is updating quickly, but it's not really ready for interaction yet. See here as an example race condition (I select an M and add it to my cart, but it comes in as a S, the previous selection, because search params hadn't updated yet):

https://github.com/vercel/commerce/assets/367936/f5cb0141-663c-4583-9a8c-9771043395ec

mhart commented 6 months ago

Ok, answering my own question here – maybe.

It might just be my misinterpretation of what useRouter does. The docs say it will "Perform a client-side navigation to the provided route".

In my mind that means: no server invocation, it's a client-side operation.

But it might be (correct me if I'm wrong) that it means: fetch RSC from the server-side to update the dom.

If I change it to use window.history.pushState instead it seems to do what I expected (and no need for useOptimistic race conditions, etc)

linkb15 commented 5 months ago

@leerob @mhart yea I am about to ask this as well, I am thinking window.history.pushState should work for changing variants right?

But I am not sure whether the API calls will be called if we do that from server component running or not.

And if we are changing the document title according to the query params pushed, is it gonna be updated or not. I have not tested this concern yet.