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

Feature/2985 - a.k.a "localStorage saver" #3180

Merged
merged 5 commits into from
Jul 10, 2019
Merged

Feature/2985 - a.k.a "localStorage saver" #3180

merged 5 commits into from
Jul 10, 2019

Conversation

pkarw
Copy link
Collaborator

@pkarw pkarw commented Jul 4, 2019

Related issues

This is related to #2985

Short description and why it's useful

I've decreased the localStorage quota usage + error handling by introducing new config variables:

  • config.products.disablePersistentProductsCache to not store products by SKU (by default it's on). Products are cached in ServiceWorker cache anyway so the product/list will populate the in-memory cache (cache.setItem(..., memoryOnly = true));
  • config.seo.disableUrlRoutesPersistentCache - to not store the url mappings; they're stored in in-memory cache anyway so no additional requests will be made to the backend for url mapping; however it might cause some issues with url routing in the offline mode (when the offline mode PWA installed on homescreen got reloaded, the in-memory cache will be cleared so there won't potentially be the url mappings; however the same like with product/list the ServiceWorker cache SHOULD populate url mappings anyway);
  • config.syncTasks.disablePersistentTaskQueue to not store the network requests queue in service worker. Currently only the stock-check and user-data changes were using this queue. The only downside it introuces can be related to the offline mode and these tasks will not be re-executed after connectivity established, but just in a case when the page got reloaded while offline (yeah it might happen using ServiceWorker; syncTasks can't be re-populated in cache from SW)

If by some reasons you wan't to have the localStorage back on for Products by SKU, Url Routes and SyncTasks - please juset set these variables back to false in your config/local.json.

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

IMPORTANT NOTICE - Remember to update CHANGELOG.md with description of your change

Contribution and currently important rules acceptance

@pkarw pkarw added this to the 1.11.0-rc.1 milestone Jul 4, 2019
@pkarw pkarw requested a review from lukeromanowicz July 4, 2019 11:45
@patzick patzick added the QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. label Jul 5, 2019
@patzick
Copy link
Collaborator

patzick commented Jul 5, 2019

Looks good, i have nothing to add:)
i set label to test it by QA on branch before merge to be sure

@pkarw pkarw changed the title Feature/2985 Feature/2985 - a.k.a "localStorage saver" Jul 5, 2019
@pkarw
Copy link
Collaborator Author

pkarw commented Jul 5, 2019

I've just changed the config property names changing the LocalStorage to Persistent. The UniversalStorage supports also IndexedDb, WebSQL so having LocalStorage in the property names could have been missleading

@alinadivante
Copy link
Collaborator

hi @pkarw I started vsf with devServiceWorkerr=true ( yarn dev:sw ) and comparing your branch to Demo I see that less items are now stored in Local Storage or Cache Storage (i suppose that it is OK). Please write me if I should check something else.

But please look also at this error from my dev console:
image

@alinadivante alinadivante added QA rejected Testers will add this label when something is still wrong and removed QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. labels Jul 8, 2019
@pkarw
Copy link
Collaborator Author

pkarw commented Jul 9, 2019

@alinadivante thanks for testing this out! Regarding this error - it's not related to the changes included in this PR. It's the reason of "QA rejected"? :-(

@alinadivante
Copy link
Collaborator

@pkarw yes, it was the reason :P I didn't notice this error before, so I assumed that your changes are causing it. If not, I think the rest is fine.

@alinadivante alinadivante removed the QA rejected Testers will add this label when something is still wrong label Jul 9, 2019
@pkarw pkarw merged commit 0c15d32 into develop Jul 10, 2019
@patzick patzick deleted the feature/2985 branch July 10, 2019 09:47
@pkarw pkarw added backport-to-1.10 QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified. labels Oct 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-1.10 QA - Ready for tests This is notification for testers, that improvement is ready to be tested and verified.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants