Skip to content
This repository has been archived by the owner on Dec 13, 2020. It is now read-only.

Dev 979 #1016

Merged
merged 3 commits into from
Jul 13, 2017
Merged

Dev 979 #1016

merged 3 commits into from
Jul 13, 2017

Conversation

wiadev
Copy link
Contributor

@wiadev wiadev commented Jul 13, 2017

Copy link
Member

@metas-ts metas-ts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @wiadev, please check out my question regarding "cache invalidation"

@@ -3,6 +3,9 @@ import axios from 'axios';

// REQUESTS

let breadcrumbsCache = {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (mind that i'm a javascript-noob): how and when is this cache discarded or invalidated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need to invalidate it. when you refresh the page, it gets cleared. when you 're navigating, it's not changed.

Copy link
Member

@teosarca teosarca Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wiadev here is my input as yet another javascript-noob :)

Loud thinking: i think the frontend side shall not cache those results at all. It shall simply call the endpoint each time user is pressing on the breadcrumb item.
Reasons:

  • we are already caching it on backend side and it would be quite difficult and error prone to invalidate all caches along the road, when a sysadm is changing the menu.
  • when and if we would want to optimize that part and save some calls, we can do browser level caching (ETag, cache control etc), so basically, when the backend menu cache is invalidated we would send a different ETag.

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@teosarca Removed the cache.

All things you told above are correct and valid. But this particular piece simply didn't get updated that often. That's why I decided to reduce requests.

Now it removed and just don't fire duplicate requests sent for the same breadcrumb items.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

koszi :)

@metas-ts metas-ts merged commit 7c72ddb into master Jul 13, 2017
@metas-ts metas-ts deleted the dev-979 branch July 13, 2017 11:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants