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.

Sign up for GitHub

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

Jump to bottom

Don't remove nulls in cache merge #411

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Don't remove nulls in cache merge #411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
846fe18
Don't remove nulls in cache merge
janicduplessis Oct 31, 2023
c5828fb
Add test
janicduplessis Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/OnyxCache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ class OnyxCache {

// lodash adds a small overhead so we don't use it here
// eslint-disable-next-line prefer-object-spread, rulesdir/prefer-underscore-method
this.storageMap = Object.assign({}, utils.fastMerge(this.storageMap, data));
this.storageMap = Object.assign({}, utils.fastMerge(this.storageMap, data, false));
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we would just leave in the storage map a bunch of keys with null and never delete them? I think it does.

It is a bit hard to wrap one's head around what the side effects of this change would be 😄

One possible problem is here:

react-native-onyx/lib/Onyx.js

Lines 125 to 129 in cd54318

function get(key) {
// When we already have the value in cache - resolve right away
if (cache.hasCacheForKey(key)) {
return Promise.resolve(cache.getValue(key));
}

Since a value that we are setting again after deleting previously would be undefined but now would be regarded as null so we would not check to see if it should be read from storage.

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 will have more null values in cache storage yes, but this is only the in memory cache and not the persisted storage so I think it is fine to speedup lookup of values that we know do not exist in storage.

The behavior for deleted values should not change since the cache has a drop method which should be used to remove values completely. The only case I know of that we remove values from cache so they will be fetched from storage again is

react-native-onyx/lib/OnyxCache.js

Line 185 in cd54318

removeLeastRecentlyUsedKeys() {
which doesn't use merge. In other cases if we use merge with nulls to signal values were deleted it is fine to have them in cache still since we know their latest value is that they do not exist.

I hope that clears up any confusion.

Copy link
Contributor

@marcaaron marcaaron Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

it is fine to speedup lookup of values that we know do not exist in storage.

That's a good point - I was thinking more along the lines of whether this could lead to anything unexpected e.g. could it somehow be possible for the value to exist in storage, but still null in the cache? But doesn't seem likely.


const storageKeys = this.getAllKeys();
const mergedKeys = _.keys(data);
Expand Down
12 changes: 12 additions & 0 deletions tests/unit/onyxCacheTest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,18 @@ describe('Onyx', () => {
expect(() => cache.merge(0)).toThrow();
expect(() => cache.merge({})).not.toThrow();
});

it('Should merge `null` values', () => {
// `null` values can override existing values and should also
// be preserved during merges.
cache.set('mockKey', {ID: 5});
cache.set('mockNullKey', null);

cache.merge({mockKey: null});

expect(cache.getValue('mockKey')).toEqual(null);
expect(cache.getValue('mockNullKey')).toEqual(null);
});
});

describe('hasPendingTask', () => {
Expand Down
Loading