-
Notifications
You must be signed in to change notification settings - Fork 300
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
Fix #11686. Trigger a cache update immediately after reading from cache. #11687
Fix #11686. Trigger a cache update immediately after reading from cache. #11687
Conversation
Codecov Report
@@ Coverage Diff @@
## main #11687 +/- ##
=======================================
Coverage 61% 62%
=======================================
Files 483 483
Lines 34362 34368 +6
Branches 5571 5573 +2
=======================================
+ Hits 21245 21562 +317
+ Misses 11018 10740 -278
+ Partials 2099 2066 -33
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please could you check the changes here #11692
I think that might be a better way, I didn't want to push changes to your PR
please use my PR as a suggestion.
I think its better to trigger the change event closer to where the values are updated,
the problem today is the getFromCache
updates the cached property when i think that's incorrect, as it should just return as the method implies, else the method should be named initializeFromCache
The reason, I'm rejecting this PR is it feels a little hackish to call updateCache
after calling getFromCache
& the fact that we're relying on the side effect for the event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Appoving the PR as my nit with the current approach doesn't warrant a rejection,
it's merely a code smell
@DonJayamanne thanks for reviewing the changes. I guess the naming of the functions is a bit confusing but the reason we need one more |
Fixes #11686.
package-lock.json
has been regenerated by runningnpm install
(if dependencies have changed).