-
Notifications
You must be signed in to change notification settings - Fork 2.1k
bugfix - revert init filters #3968
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 - revert init filters #3968
Conversation
pkarw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gibkigonzo it will cause memory leak - there is a known issue with Vue.mixins and Vue.filters and memory leaks
if we're to move it out this once helper then it must be executed ONLY in CSR mode - however not sure if it won't cause another side effects blinking or CSR/SSR DOM mismatch
Reference: vuejs/vue#5089
|
I just reverted this from 1.10.5. It's wierd problem because if we call those initialization in |
|
@pkarw Problem is that we add filters once and then in |
|
Or we could pass storeView to price filter (and date filter). Filters would be more pure, but we would have to pass this every time in template |
pkarw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. I'd opt for both solutions:
- use the
globalfor thestoreViewplaceholder to keep b.c. - use it as a filter parameter (with default value pointing to this global) - to be used in the new themes
| export function currentStoreView (): StoreView { | ||
| // TODO: Change to getter all along our code | ||
| return rootStore.state.storeView | ||
| const serverStoreView = get(global, 'process.storeView', undefined) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about client-side? there is no global?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no global on client-side, I could use window, but I think we can stay with same behavior as it was for client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is in next line const clientStoreView = get(rootStore, 'state.storeView', undefined)
|
@gibkigonzo one comment left. Is it ready? |
|
@pkarw @andrzejewsky I've applied suggestion |
Short Description and Why It's Useful
On init load https://test.storefrontcloud.io/women/women-20 filters are missing on SSR so we get price without format.
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