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

Bugfix/multiple local cache load for wishlist and compare #2431

Conversation

patzick
Copy link
Collaborator

@patzick patzick commented Feb 13, 2019

Short description and why it's useful

Ensure that wishlist and compare module read only once from local cache.

Screenshots of visual changes before/after (if there are any)

Problem happened on product page, because there is couple of wishlist and compare components.

Before:
image

After:
image

Upgrade Notes and Changelog

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

@patzick patzick added this to the 1.9 milestone Feb 13, 2019
@patzick patzick requested a review from pkarw February 13, 2019 11:51
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.

@patzick the question is why “load” action is called multiple times? Isn’t it strange?

@pkarw
Copy link
Collaborator

pkarw commented Feb 13, 2019

If this is the only ultimate solution then we should add the same to “cart” module as well :)

@patzick
Copy link
Collaborator Author

patzick commented Feb 13, 2019

@pkarw that's because load action reads from local cache and have to be invoked on mounted event. It was solved that way to prevent SSR problems. So every module component implements common module mixin which dispatches load action on mounted lifecycle. So data are loaded after component is rendered.
Thats why we need that flag if we don't want to get data from local cache.

But i don't get it why we should now add it to the cart module as well.

@pkarw
Copy link
Collaborator

pkarw commented Feb 13, 2019

Ok, now I got it. It would be much better to have more general solution maybe we can deal with this in this general mixin/ having Singleton like behavior or whatever? I mean: it’s easy to forget about this flagging when creating a new module like compare - our core modules should be kind of “best practices” and this solution looks a little bit like workaround than best practice :-)

Wdyt?

@patzick
Copy link
Collaborator Author

patzick commented Feb 13, 2019

Yeah i thought about it a lot, but it's not that obvious. My first thought was to just add mounted on App.vue component and then we could create hook/event/whatever to just connect to it. But unfortunately when i tested it mounted event on App doesn't mean that other components are rendered too, so it called this to early and either way we had ssr mismatch problem.
And i haven't found yet a best way to detect if app is fully mounted so thats why i came up with this mixin idea in module components. I'm sure there has to be a way to detect that app is mounted, or even better - there would be great if we could know when hydration process is finished. Because this point is critical and would solve all our problems with SSR and local cache. I hope that i finally will find a way to detect that.

@pkarw
Copy link
Collaborator

pkarw commented Feb 13, 2019

In that case, we should at least create some hook (I mean - re-usable behavior like React hooks or mixin) or whatever to simply have a tool for further modules like this something You can invoke in Your Vuex action to make sure the logic is executed just once - something like Singleton / Mutex in multithreading

Maybe we could use AsyncDataLoader for this purpose adding sth like AsyncDataLoader.once()

This is just an idea. Please fix this issue like You were doing framework not like a case-by-case hotfix :)

@filrak
Copy link
Collaborator

filrak commented Feb 13, 2019

@pkarw imo the problem is not about invoking it multiple times but about invoking it after hydration. We just need a way to know when it happens (@patzick is already trying to figure it out). Once we figure it out we will think about proper API to introduce it (probably as a part of before/after hook signature as you suggested like VSF.onHydrationReady() or sth like this)

@pkarw
Copy link
Collaborator

pkarw commented Feb 18, 2019

Hydration is executed in the client-entry ($mount).
I like the idea with the “onAfterHydration” event or trying to move this loading logic to native 2.6.x “serverPrefetch” (?) as we like to keep components simpler and leaner I like more the onAfterHydration @filrak @patzick please fix this in the current stabilization sprint

@patzick
Copy link
Collaborator Author

patzick commented Feb 18, 2019

@pkarw when we discover a way we'll make hook for this. Currently i believe this is the best way to deal with this problem. Vue 2.6 "serverPrefetch" is not solving this issue - it just allows to do asyncData in any component, so still you have no access to localStorage and will break hydration calling it there.
I've ask Vue team members about that and waiting for response, but for now this working solution.

P.S. i've also tried your $mounted idea with no positive results

@pkarw pkarw requested a review from filrak February 18, 2019 19:50
@filrak
Copy link
Collaborator

filrak commented Feb 20, 2019

@patzick maybe let's keep the PR open and figure out the API for a proper hook before the stabilization phase. I have some ideas, let's discuss them ;)

@patzick
Copy link
Collaborator Author

patzick commented Feb 21, 2019

I have response from Vue core team. Evan said that currently there is no way to detect that hydration is completed. His suggestion is to add changes to components - static and lazy loaded.
So that is exactly what is implemented here. In the future, when we'll find a reliable solution then we will create a hook for that.

@pkarw
Copy link
Collaborator

pkarw commented Feb 21, 2019

Ok!

@pkarw
Copy link
Collaborator

pkarw commented Feb 26, 2019

I think You can safely merge this one in :-) @filrak @patzick

@patzick patzick modified the milestones: 1.9, 1.8.3 Feb 26, 2019
@patzick patzick merged commit aa30ecc into vuestorefront:develop Feb 26, 2019
@patzick patzick deleted the bugfix/multiple-local-cache-load-for-wishlist-and-compare branch February 26, 2019 18:18
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

Successfully merging this pull request may close these issues.

None yet

3 participants