-
Notifications
You must be signed in to change notification settings - Fork 95
Prevent updating values in cache when they do not change #191
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
Changes from all commits
23a135c
5e93a95
483ff70
0e63c88
8c7b7d9
2fbf183
1ee97da
17e0903
d049e6d
194e555
b24806b
72feff5
0181cf0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -367,11 +367,15 @@ function keysChanged(collectionKey, partialCollection) { | |||||
| /** | ||||||
| * When a key change happens, search for any callbacks matching the key or collection key and trigger those callbacks | ||||||
| * | ||||||
| * @example | ||||||
| * keyChanged(key, value, subscriber => subscriber.initWithStoredValues === false) | ||||||
| * | ||||||
| * @private | ||||||
| * @param {String} key | ||||||
| * @param {*} data | ||||||
| * @param {Function} [canUpdateSubscriber] only subscribers that pass this truth test will be updated | ||||||
| */ | ||||||
| function keyChanged(key, data) { | ||||||
| function keyChanged(key, data, canUpdateSubscriber) { | ||||||
| // Add or remove this key from the recentlyAccessedKeys lists | ||||||
| if (!_.isNull(data)) { | ||||||
| addLastAccessedKey(key); | ||||||
|
|
@@ -385,7 +389,7 @@ function keyChanged(key, data) { | |||||
| const stateMappingKeys = _.keys(callbackToStateMapping); | ||||||
| for (let i = stateMappingKeys.length; i--;) { | ||||||
| const subscriber = callbackToStateMapping[stateMappingKeys[i]]; | ||||||
| if (!subscriber || !isKeyMatch(subscriber.key, key)) { | ||||||
| if (!subscriber || !isKeyMatch(subscriber.key, key) || (_.isFunction(canUpdateSubscriber) && !canUpdateSubscriber(subscriber))) { | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NAB: Is this new logic the same thing as using Maybe not... I think I am thinking of this: https://github.com/Expensify/expensify-common/blob/main/lib/Func.jsx#L13-L19 which would be cool if we could just import it without any additional work :D I don't think that's possible though.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What you are asking sounds similar to
and would return |
||||||
| continue; | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -631,12 +635,16 @@ function disconnect(connectionID, keyToRemoveFromEvictionBlocklist) { | |||||
| * available async. Since we have code in our main applications that might expect things to work this way it's not safe to change this | ||||||
| * behavior just yet. | ||||||
| * | ||||||
| * @example | ||||||
| * notifySubscribersOnNextTick(key, value, subscriber => subscriber.initWithStoredValues === false) | ||||||
| * | ||||||
| * @param {String} key | ||||||
| * @param {*} value | ||||||
| * @param {Function} [canUpdateSubscriber] only subscribers that pass this truth test will be updated | ||||||
| */ | ||||||
| // eslint-disable-next-line rulesdir/no-negated-variables | ||||||
| function notifySubscribersOnNextTick(key, value) { | ||||||
| Promise.resolve().then(() => keyChanged(key, value)); | ||||||
| function notifySubscribersOnNextTick(key, value, canUpdateSubscriber) { | ||||||
| Promise.resolve().then(() => keyChanged(key, value, canUpdateSubscriber)); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -703,6 +711,14 @@ function set(key, value) { | |||||
| Logger.logAlert(`Onyx.set() called after Onyx.merge() for key: ${key}. It is recommended to use set() or merge() not both.`); | ||||||
| } | ||||||
|
|
||||||
| // If the value in the cache is the same as what we have then do not update subscribers unless they | ||||||
| // have initWithStoredValues: false then they MUST get all updates even if nothing has changed. | ||||||
| if (!cache.hasValueChanged(key, value)) { | ||||||
| cache.addToAccessedKeys(key); | ||||||
| notifySubscribersOnNextTick(key, value, subscriber => subscriber.initWithStoredValues === false); | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Why does the third argument need to be a function? Can it be a simple boolean instead?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't have to be a function. I had it as a simple boolean, but feel the function is more intuitive and also puts the context for why we are doing what we are doing where we are doing it. Check this commit to get a feel for what I mean: |
||||||
| return Promise.resolve(); | ||||||
| } | ||||||
|
|
||||||
| // Adds the key to cache when it's not available | ||||||
| cache.set(key, value); | ||||||
| notifySubscribersOnNextTick(key, value); | ||||||
|
|
||||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.