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

Performance levers around router.push and Suspense #1334

Open
mhart opened this issue May 7, 2024 · 5 comments
Open

Performance levers around router.push and Suspense #1334

mhart opened this issue May 7, 2024 · 5 comments

Comments

@mhart
Copy link

mhart commented May 7, 2024

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
Copy link
Member

leerob commented May 8, 2024

The answer is here! #1327

@mhart
Copy link
Author

mhart commented May 8, 2024

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
Copy link
Author

mhart commented May 8, 2024

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):

Screen.Recording.2024-05-08.at.2.00.34.PM.mov

@mhart
Copy link
Author

mhart commented May 8, 2024

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
Copy link

linkb15 commented May 28, 2024

@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.

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

No branches or pull requests

3 participants