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

Fix isResourceEnabled jank using a cache #10289

Merged
merged 1 commit into from
Aug 24, 2017
Merged

Fix isResourceEnabled jank using a cache #10289

merged 1 commit into from
Aug 24, 2017

Conversation

diracdeltas
Copy link
Member

@diracdeltas diracdeltas commented Aug 3, 2017

fix #9987

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plan:

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@diracdeltas diracdeltas requested a review from bbondy August 3, 2017 23:22
@diracdeltas diracdeltas self-assigned this Aug 4, 2017
@diracdeltas diracdeltas added this to the 0.19.x (Beta Channel) milestone Aug 4, 2017
@luixxiul luixxiul modified the milestones: 0.21.x (Nightly Channel), 0.19.x (Beta Channel) Aug 7, 2017
app/filtering.js Outdated
}

const settingsKeys = ['siteSettings', 'temporarySiteSettings', 'settings']
const appStoreChangeCallback = function (diffs) {
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of an app store listener on all events here we should handle the events that change site settings in a reducer and call into a cache module (braverySettingsCache) managed from the reducer and used in filtering.

@diracdeltas
Copy link
Member Author

@bbondy i have updated the PR

@codecov-io
Copy link

codecov-io commented Aug 15, 2017

Codecov Report

Merging #10289 into master will decrease coverage by 0.01%.
The diff coverage is 40.54%.

@@            Coverage Diff             @@
##           master   #10289      +/-   ##
==========================================
- Coverage   54.52%    54.5%   -0.02%     
==========================================
  Files         244      245       +1     
  Lines       21129    21157      +28     
  Branches     3262     3263       +1     
==========================================
+ Hits        11520    11532      +12     
- Misses       9609     9625      +16
Flag Coverage Δ
#unittest 54.5% <40.54%> (-0.02%) ⬇️
Impacted Files Coverage Δ
js/stores/appStore.js 12.3% <0%> (-0.16%) ⬇️
js/settings.js 98.46% <100%> (+1.53%) ⬆️
app/filtering.js 18.22% <11.76%> (+0.15%) ⬆️
app/common/cache/braverySettingsCache.js 60% <60%> (ø)

@NejcZdovc NejcZdovc modified the milestones: 0.19.x (Beta Channel), 0.21.x (Nightly Channel) Aug 15, 2017
@bbondy bbondy modified the milestones: 0.18.x Hotfix (Release Channel), 0.19.x (Beta Channel) Aug 23, 2017
@diracdeltas
Copy link
Member Author

@bbondy updated PR

@bbondy bbondy merged commit 661ef90 into master Aug 24, 2017
bbondy added a commit that referenced this pull request Aug 24, 2017
Fix isResourceEnabled jank using a cache
bbondy added a commit that referenced this pull request Aug 24, 2017
Fix isResourceEnabled jank using a cache
bbondy added a commit that referenced this pull request Aug 24, 2017
Fix isResourceEnabled jank using a cache
@bsclifton bsclifton deleted the fix/9987 branch November 2, 2017 06:15
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.

Reduce page load jank from isResourceEnabled checks
5 participants