Feat. Collection - add pagination to limit the amount of fetched data #3688
Conversation
There was a problem hiding this comment.
Great change @ludoboludo! Let me add a little bit of context for anyone interested.
One of the most impactful Liquid performance tips is to make sure the code does as few iterations as possible. You don't want to waste any cycles going through the data that you don't actually need.
I used to think that having a limit: in the for loop should be enough to achieve this. You should still use it, but it's only a half of the equation.
I stumbled upon this issue when optimising one of the stores. It was suffering from high server response times on homepage. The layout was quite simple, all featured collection sections already were using the limit: so I had to dig deeper into what's going on.
After consulting our Liquid folks, I learned that all collection.products objects follow the default pagination rules. By default, they will fetch the first 50 products in the given collection. This is not an issue for small or simple catalogues, but can lead to longer queries when you have a lot of variants, prices, etc.
All in all, when iterating over a collection and using limit, you should also use the paginate as well. The former will limit the amount of iteration, the latter makes sure you're not over-fetching the data.
There was a problem hiding this comment.
For my own understanding: do we need to use limit on the forloop once we have a paginate in place?
From @krzksz's comment it sounds like paginate will fetch only products_to_show items, that we then iterate on, so we don't risk running too many iterations already?
Or are we saying that the for loop will run through the whole collection but paginate won't output it all?
sections/featured-collection.liquid
Outdated
|
|
||
| {%- if section.settings.collection.products.size > 0 -%} | ||
| {% paginate section.settings.collection.products by section.settings.products_to_show %} | ||
| {%- for product in section.settings.collection.products limit: section.settings.products_to_show -%} |
There was a problem hiding this comment.
I might be confused, but isn't using the paginate tag already giving us the "limit"?
There was a problem hiding this comment.
That's a good point 🤔 In this context it doesn't seem like the limit would be needed if the pagination alone is already limiting the amount of data fetched.
cc: @krzksz
There was a problem hiding this comment.
It might be a happy accident / unintended result of how these tags work and not best practice; either way just trying to understand these 2 tags better 😁
There was a problem hiding this comment.
Updated 👍 Removed the limit from the for loop since like mentioned it's already being paginated.
PR Summary:
Wraps the forloop on the feat. collection section to limit the amount of fetched data, knowing we're only needing as many as our limit is suggesting.
Why are these changes introduced?
Fix concerns about performance.
What approach did you take?
Updated the forloop to be wrapped in a if statement.
Other considerations
Visual impact on existing themes
Shouldn't have any
Testing steps/scenarios
Demo links
Checklist