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

Adding template generation timestamp to cache key, so the /navigation/mobile/index/ can be cached on client #3

Closed
jtomaszewski opened this issue Dec 7, 2020 · 4 comments

Comments

@jtomaszewski
Copy link

jtomaszewski commented Dec 7, 2020

Currently on each store visit GET /navigation/mobile/index/?cache_key=... request is done from the client which returns HTTP 200.

Since it already includes cache key, why don't we make it return 304, so it can be cached forever on the client, to make its' response even faster?

However, we would need to add "template generation timestamp" to the MageSuite\Navigation\Block\Navigation.getCacheKeyInfo(), which could be cached between requests in in-memory cache (redis), so that the cache key is changed when bin/magento cache:clean is done (e.g. after categories tree is changed).

WDYT? Would u accept PR for that?

@diwipl
Copy link
Contributor

diwipl commented Dec 7, 2020

Hi

Thank you for bug report.

Navigation result should be cached inside local storage. Request should be done only once per cache key.

Seems like we might have some bug here with caching.

@diwipl
Copy link
Contributor

diwipl commented Dec 7, 2020

Ok, I just got confirmation from frontend devs that we no longer cache in local storage because of issues with some browsers.

We rely on varnish caching so there should not be any significant load on backend servers.

@jtomaszewski
Copy link
Author

Ok, I just got confirmation from frontend devs that we no longer cache in local storage because of issues with some browsers.

I see. I guess it was because of browsers' local storage memory size limitation?

@diwipl
Copy link
Contributor

diwipl commented Dec 7, 2020

Yes, exactly.

Some of our customers have quite extensive navigations and it caused problems.

@diwipl diwipl closed this as completed Dec 11, 2020
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

No branches or pull requests

2 participants