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

Introduce chrome cache #959

Merged
merged 3 commits into from
Sep 15, 2020

Conversation

karelhala
Copy link
Contributor

No description provided.

@karelhala karelhala force-pushed the introduce-chrome-cache branch 5 times, most recently from 5508479 to 63a7547 Compare September 14, 2020 12:30
@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2020

Codecov Report

Merging #959 into master will increase coverage by 1.56%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #959      +/-   ##
==========================================
+ Coverage   51.12%   52.69%   +1.56%     
==========================================
  Files          56       57       +1     
  Lines        1021     1057      +36     
  Branches      200      204       +4     
==========================================
+ Hits          522      557      +35     
- Misses        396      397       +1     
  Partials      103      103              
Impacted Files Coverage Δ
src/js/App/Sidenav/SideNav.js 23.07% <ø> (ø)
src/js/entry.js 25.37% <0.00%> (-0.39%) ⬇️
src/js/jwt/jwt.js 81.81% <ø> (ø)
src/js/nav/globalNav.js 74.19% <0.00%> (-7.95%) ⬇️
src/js/utils.js 45.23% <0.00%> (+8.02%) ⬆️
src/js/utils/cache.js 96.96% <96.96%> (ø)

@karelhala karelhala marked this pull request as ready for review September 14, 2020 12:42
export async function loadNav(yamlConfig) {
const groupedNav = await getNavFromConfig(safeLoad(yamlConfig));
export async function loadNav(yamlConfig, cache) {
let groupedNav = await cache.getItem('navigation');
Copy link
Contributor

Choose a reason for hiding this comment

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

@karelhala when is this cache supposed to be populated? I see this undefined on each page refresh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Hyperkid123 mhh, that is strange. It should be populated from entry.js I checked it locally and it looks populated.

Copy link
Contributor

@Hyperkid123 Hyperkid123 Sep 14, 2020

Choose a reason for hiding this comment

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

What key is supposed to be stored in localSotarge? I have it stored under chrome:global-filter/chrome. I don't see anything stored under key with navigation string. Is it possible that your code might be using stale storage? Can you try and clear it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, tried clearing cache and it still works, you should be able to find it under ${session}-chrome/chrome where expires is set and in data you should see navigation. I don't want to polute localStorage more than we did so I think we could use one localForge store for all chrome related non API data. I'd like to add global filter values to it once we agree on the shape of it and such.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I have in local storage

chrome:global-filter/6e9e3e69-7364-4bbc-94c6-45e2f5ffe7c6: "{}"
chrome:global-filter/chrome: "{"data":{"navigation":{"ansible":{"title":"Red Hat Ansible Automation Platform","id":"ansible","routes":[{"tit

If I log the cache name from the nav should be extracted its chrome:global-filter/6e9e3e69-7364-4bbc-94c6-45e2f5ffe7c6 which is empty object in my local storage.

Copy link
Contributor

@Hyperkid123 Hyperkid123 Sep 14, 2020

Choose a reason for hiding this comment

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

@karelhala ok it works now. I had to logout and login to make it work. 🤷 possible bug? The key selection seems "unreliable"

@karelhala karelhala merged commit 54e888a into RedHatInsights:master Sep 15, 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

Successfully merging this pull request may close these issues.

3 participants