Fix adding unconfigured products to compare & optimize fetching homepage data #3512
Conversation
…n homepage, remove unused cool bags code
There was a problem hiding this comment.
Just small changes from me :) And please update changelog.
Love that store for the homepage is typically on theme part! I'd be voting for removing rest of queries from config like bestSellers and move them directly to actions or as action parameters. It's easier for developer to modify and find them. But it's not for this PR, just leaving this here ;D
src/themes/default/store/homepage.ts
Outdated
| export const homepageStore = { | ||
| namespaced: true, | ||
| state: { | ||
| new_collection: [] | ||
| }, | ||
| actions: { | ||
| fetchNewCollection ({ commit, dispatch }) { |
There was a problem hiding this comment.
| fetchNewCollection ({ commit, dispatch }) { | |
| async fetchNewCollection ({ commit, dispatch }) { |
There was a problem hiding this comment.
please apply this one as well, all actions should return a promise by default
src/themes/default/store/homepage.ts
Outdated
| fetchNewCollection ({ commit, dispatch }) { | ||
| const newProductsQuery = prepareQuery({ queryConfig: 'newProducts' }) | ||
|
|
||
| dispatch('product/list', { |
There was a problem hiding this comment.
please just change this to async/await
There was a problem hiding this comment.
@patzick awaits are usually slower, that's why I avoid them. Dispatch should be returned though, I give you that.
There was a problem hiding this comment.
@lukeromanowicz we use babel to transpile to ES5 anyway, async/await highly increase readability and it's a convention we agreed to use.
Btw. please show me the studies about performance difference, you mentioned here, it's very interesting :)
There was a problem hiding this comment.
I also would like to see such studies since all I ever read was saying it's still a microtask under the hood and just syntax sugar for Promises ;)
It can be slower only if you don't know how it works (not concurrently) and expect multiple await to run simultaneously but it's exactly the same with Promises
There was a problem hiding this comment.
@patzick https://v8.dev/blog/fast-async it was this one
src/themes/default/pages/Home.vue
Outdated
| @@ -70,9 +66,6 @@ export default { | |||
| everythingNewCollection () { | |||
| return this.$store.state.homepage.new_collection | |||
There was a problem hiding this comment.
would be perfect to add a getter for newCollection in store and use it here:)
There was a problem hiding this comment.
Right 👍 I guess I can refactor that part as well.
src/themes/default/store/homepage.ts
Outdated
| export const homepageStore = { | ||
| namespaced: true, | ||
| state: { | ||
| new_collection: [] | ||
| }, | ||
| actions: { | ||
| fetchNewCollection ({ commit, dispatch }) { |
There was a problem hiding this comment.
please apply this one as well, all actions should return a promise by default
…ub.com:lukeromanowicz/vue-storefront into bugfix/adding-unconfigured-products-to-compare
fix adding unconfigured products to compare, optimize fetching data on homepage, remove unused cool bags code
Related issues
closes #3293
Which environment this relates to
Check your case. In case of any doubts please read about Release Cycle
developbranch and want to merge it back todevelopreleasebranch and want to merge it back toreleasehotfixormasterbranch and want to merge it back tohotfixUpgrade Notes and Changelog
IMPORTANT NOTICE - Remember to update
CHANGELOG.mdwith description of your changeContribution and currently important rules acceptance