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

Apply lazy-hydrate on category page #3344

Conversation

andrzejewsky
Copy link
Contributor

Related issues

closes #3327

Short description and why it's useful

This is a PoC of introducing lazy-hydrate for category pages. I had to face many issues with that. First of all the goal was to remove category-next.products from __INITIAL_STATE__ but it causes lack of reactivity in the vuex so the products field will not be refreshed. Second one - SSR rendering - the asyncData is calling before components were rendered and it stores the products in the vuex state that you have deleted beforehand.... and even more...

let me explain what I did and why:

  1. In order to keep reactivity we can't just remove fields from the state - I've introduced filterInitialState which uses new option in the config initialStateFilterDefaults that includes fields that will be set to their default values I used new option because we might need a possibility to remove totally some field, so initialStateFilter still works as before.
  2. Because we have deleted (set to default) products by the filter (initialStateFilterDefaults), so there is no data in the state but we actually have fetched products so... I stored them in the just ordinary variable ssrProducts and if it's a SSR, it uses this variable, otherwise - read the data from getter (computed property categoryProducts)

The rest of code should be understandable. I know it's a bit hacky not it does work. I've not noticed a big difference in the lighthouse, but I tested this only on the dev environment. I have to admit that it reduces page size by ~40-44% though.

Let me know guys what you think.

Which environment this relates to

  • Test version (https://test.storefrontcloud.io) - this is a new feature or improvement for Vue Storefront. I've created branch from develop branch and want to merge it back to develop
  • RC version (https://next.storefrontcloud.io) - this is a stabilisation fix for Release Candidate of Vue Storefront. I've created branch from release branch and want to merge it back to release
  • Stable version (https://demo.storefrontcloud.io) - this is an important fix for current stable version. I've created branch from hotfix or master branch and want to merge it back to hotfix

Upgrade Notes and Changelog

  • No upgrade steps required (100% backward compatibility and no breaking changes)
  • I've updated the Upgrade notes and Changelog on how to port existing VS sites with this new feature

Contribution and currently important rules acceptance

@pkarw pkarw requested a review from patzick August 9, 2019 05:25
@pkarw
Copy link
Collaborator

pkarw commented Aug 9, 2019

@patzick and @filrak take a look at this one. It’s probably our way to beat Lighthouse 👍

Copy link
Collaborator

@pkarw pkarw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to deal with ssrProducts local variable - remove it from Category.vue leve; otherwise it's cool

src/themes/default/pages/Category.vue Outdated Show resolved Hide resolved
src/themes/default/pages/Category.vue Show resolved Hide resolved
Copy link
Collaborator

@filrak filrak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly

  • initialStateFilter removes certain STORES from Vuex completely
  • initialStateFilterDefaults sets certain FIELDS to their initial values, right?

I'm wondering if we could somehow merge this functionalities and always set defaults for fields in initialStateFilter. Amount of additional code will be barely noticeable and won't affect performance at all. in return we will cut off the complexity

@andrzejewsky
Copy link
Contributor Author

andrzejewsky commented Aug 9, 2019

@filrak

If I understand correctly

  • initialStateFilter removes certain STORES from Vuex completely
  • initialStateFilterDefaults sets certain FIELDS to their initial values, right?

Yeah this is how it works.

You are right, we can set always initial values instead of removing them. I did this in that way just because I wanted to avoid potential errors / BC-breaks, but I'll check that later.

@andrzejewsky andrzejewsky force-pushed the feature/3327-apply-vue-lazy-hydrate-category-page branch 3 times, most recently from 39a0350 to 34aa9bf Compare August 10, 2019 18:26
@andrzejewsky
Copy link
Contributor Author

Ok, I've spent some time on it and I even removed this local-variable-way hack @pkarw . Actually everything works via vuex (getters). It was possible because I've changed the way of the filtering __INITIAL_STATE__. Now, the state filter is calling just before the output render (sending to the client), so the SSR application is able to use fully vuex - beforehand it was a problem because sometimes we were removing the state fields when the application wanted to read something from the state (this is what I've noticed). Now, the initialStateFilter sets indicated fields to their default values (as you mentioned @filrak )

@andrzejewsky andrzejewsky changed the title [PoC] Apply lazy-hydrate on category page Apply lazy-hydrate on category page Aug 10, 2019
@andrzejewsky andrzejewsky added not ready for merge PR is holded. Needs some clarifications or things that need to be finished. QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. labels Aug 10, 2019
@pkarw
Copy link
Collaborator

pkarw commented Aug 12, 2019

OK, for me it's fine! we need some @alinadivante on QA + @patzick final approval

@pkarw pkarw removed the not ready for merge PR is holded. Needs some clarifications or things that need to be finished. label Aug 12, 2019
@pkarw
Copy link
Collaborator

pkarw commented Aug 13, 2019

@andrzejewsky just one more thing :-) can we enable this lazy loading feature only if config variable like: config.products..useLazyLoading?

@alinadivante
Copy link
Collaborator

I started testing it today, but I will continue on Friday. Just write here if any others changes will be applied and what I should take into account during testing :)

@andrzejewsky andrzejewsky force-pushed the feature/3327-apply-vue-lazy-hydrate-category-page branch 3 times, most recently from 2b03048 to 221e78f Compare August 13, 2019 20:17
@andrzejewsky andrzejewsky force-pushed the feature/3327-apply-vue-lazy-hydrate-category-page branch from 221e78f to 261b15c Compare August 13, 2019 20:19
@andrzejewsky
Copy link
Contributor Author

@pkarw I added option config.products.lazyLoadingCategoryProducts. It enables lazy-hydrate component and puts category-next.products to the filter list (because this option doesn't make sense without that filter)

@pkarw
Copy link
Collaborator

pkarw commented Aug 14, 2019

Cool, thanks! Waiting for @alinadivante feedback before we merge it in

@andrzejewsky andrzejewsky force-pushed the feature/3327-apply-vue-lazy-hydrate-category-page branch from 5887099 to 261b15c Compare August 16, 2019 10:59
@alinadivante
Copy link
Collaborator

for me it works good :)
I didn't notice anything wrong

@alinadivante alinadivante removed the QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. label Aug 16, 2019
@alinadivante alinadivante added the QA approved on branch Testers will add this label after positive check on specific branch. label Aug 16, 2019
@pkarw
Copy link
Collaborator

pkarw commented Aug 16, 2019

Ok! I’m merging it in

@pkarw pkarw merged commit 1955c4b into vuestorefront:develop Aug 16, 2019
@pkarw
Copy link
Collaborator

pkarw commented Aug 16, 2019

So @andrzejewsky the next challenge will be to optimize the product page ;) but first @patzick must finish his refactoring effort on it

@pkarw
Copy link
Collaborator

pkarw commented Aug 16, 2019

Great job! Thanks!

@filrak filrak self-assigned this Sep 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA approved on branch Testers will add this label after positive check on specific branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants