[HOLD] Don't include null in collection callbacks by roryabraham · Pull Request #311 · Expensify/react-native-onyx

roryabraham · 2023-08-22T05:48:02Z

Details

Coming from Expensify/App#25481 (comment), we are including null in Onyx.connect subscriber callbacks if it's a collection key w/ waitForCollectionCallback: true 🐛

This PR adds a test to cover this edge case and then prevents it such that the full collection w/o null is given to the subscriber.

Related Issues

Fixes #310

Automated Tests

Added automated test to cover this.

Linked PRs

Not created yet

Pujan92 · 2023-08-22T07:10:12Z

-                cachedCollection[key] = data;
+                if (data !== null) {
+                    cachedCollection[key] = data;
+                }


For data null, don't we need to remove that key from the cachedCollection as we are not updating with any other value?

I agree. Gonna ask the question another way:

Why do we need to set data to the key for the cachedCollection? Shouldn't the cache already be correct by this point? If we are adding a key it should be there. If we are removing a key it should not be there.

Added assertions to ensure that keys that are being deleted due to being null are dropped from both cache and storage.

marcaaron · 2023-08-22T19:43:03Z

-                cachedCollection[key] = data;
+                if (data !== null) {
+                    cachedCollection[key] = data;
+                }


I agree. Gonna ask the question another way:

Why do we need to set data to the key for the cachedCollection? Shouldn't the cache already be correct by this point? If we are adding a key it should be there. If we are removing a key it should not be there.

dangrous · 2023-08-24T16:05:26Z

I have reviewed but have no further comments outside of what's already been commented, so will hold for the final set once those qs are addressed!

…x.connect when data is missing

roryabraham · 2023-09-01T14:47:55Z

    }

-    if (_.isFunction(mapping.callback)) {
+    if (_.isFunction(mapping.callback) && (isCollectionKey(mapping.key) || matchedKey)) {


This change prevents extra calls to Onyx.connect subscribers with value null and key undefined. withOnyx waits for all connection callbacks to have been called once before rendering, so that's why we added this type of callback. However, I don't think that this type of callback is helpful to other library code using Onyx.connect directly, so I prevented that here. withOnyx callbacks are handled in the block above which terminates with an early return.

Can you clarify which call to sendDataToConnection() we are trying to prevent and why? There are only a few of them - so I worry that we are adding this conditional exception in the wrong place. And maybe don't really need this change at all.

Yes, if you do this:

const myCallback = () => {}; Onyx.connect({ key: 'someKey', callback: myCallback, });

then myCallback will always first be called with null, undefined for the value and key. I think this is counter-intuitive and unhelpful in most cases.

The reason we do that is because withOnyx waits for each Onyx.connect callback to be called at least once before rendering the child component. So even if the data is missing, we call it with null, undefined.

My thinking is that by doing this we're exposing an internal implementation detail of withOnyx unnecessarily, and in the general case Onyx.connect should only execute the callback when there's some data in the store with the given key.

Sounds like the main premise in your argument is that "no Onyx.connect() subscribers need this".

thought: passing null also provides a way for an Onyx.connect() subscriber to see that it initialized without a value from storage.

Is it helpful to know this? Do we have existing code that depends on this information? I think it's safer to assume we do than don't, but also don't want to audit the 446 usages 😄

roryabraham · 2023-09-01T14:49:39Z

-                expect(mockCallback).toHaveBeenCalledTimes(2);
-
-                // AND the value for the first call should be null since the collection was not initialized at that point
-                expect(mockCallback).toHaveBeenNthCalledWith(1, null, undefined);


This is an instance of the null, undefined callback mentioned above that I removed.

marcaaron · 2023-09-01T20:30:25Z

 * @param {Function} [canUpdateSubscriber] only subscribers that pass this truth test will be updated
 */
-function keyChanged(key, data, canUpdateSubscriber) {
+function keyChanged(key, data, canUpdateSubscriber = () => true) {


In the previous case we were not calling a function unless it exists. Now we always call a function that can only return true when it doesn't exist. I think it was more obvious before and this was not really needed.

I agree this wasn't really needed for this PR, but I disagree that it was more obvious before...

Before:

param can be function or undefined

no default for an optional param. in order to understand the default behavior, you need to read the function and see how it's implemented

After:

param is always a function

obvious default for an optional param. can be understood just by looking at the function definition

I'd lean towards this new version as well, as I agree it's more understandable at a glance!

it's not a hill I will die on. I will simply change it back to what I prefer in my next Onyx PR

marcaaron · 2023-09-01T21:12:49Z

    }

-    if (_.isFunction(mapping.callback)) {
+    if (_.isFunction(mapping.callback) && (isCollectionKey(mapping.key) || matchedKey)) {


Can you clarify which call to sendDataToConnection() we are trying to prevent and why? There are only a few of them - so I worry that we are adding this conditional exception in the wrong place. And maybe don't really need this change at all.

marcaaron · 2023-09-01T21:15:36Z

 function applyMerge(existingValue, changes) {
+    if (_.some(changes, change => _.isNull(change))) {
+        return null;
+    }


This seems wrong to me. If any of the changes in the merge queue array are null then we null out the resulting value?

I am not 100% sure about this one, but I think you might be right. i.e: if I'm trying to merge all these changes into an empty object:

[ {a: 1}, {b: 2}, null, {c: 3}, ]

then what should the result be? This code makes it null, but maybe it should actually be applied in order such such that the final result is {c: 3}?

Maybe we just skip null, unless it's the last update, in which case the final result of the whole thing will be null.

Made this change, and just to clarify the change in behavior made by this PR:

Before:

If null is the last item in changes, null is the final result

If null is anywhere else in the array, only the last item in the array will be applied. So merging this into an empty object:

[ null, {a: 1}, {b: 2}, {c: 3}, ]

the result today would be {c: 3}

After:

If null is the last item in changes, null is the final result.

If null is anywhere else in the array, it is ignored.

then what should the result be?

It should be the last value in the array

If null is anywhere else in the array, only the last item in the array will be applied.

Not anywhere else in the array. Specifically, it should clear out the items that came before it. The rest should merge normally.

If null is anywhere else in the array, it is ignored.

I think this logic is wrong. It should be clearing the previous values called in the same tick of the event loop.

If we do this:

Onyx.merge('test', {a: 1}); Onyx.merge('test', null); Onyx.merge('test', {b: 2}); Onyx.merge('test', {c: 3});

Then the expected result would be {b:2, c: 3}?

marcaaron · 2023-09-01T21:22:58Z

+                // If the data is null, remove it from cache and store
+                if (_.isNull(modifiedData)) {
+                    return remove(key);
+                }


I think this can happen sooner. If the batchedChanges are null then we don't need to concern ourselves with the existingValue because we are deleting everything anyway.

marcaaron · 2023-09-01T21:28:03Z

+            _.each(collection, (value, key) => {
+                if (value === null) {
+                    keysToRemove.push(key);
+                    delete collectionWithoutKeysToRemove[key];


Rather than delete the keys it might be safer to add the ones we are keeping to a new object and avoid mutating the object passed to mergeCollection() (I think maybe we are mutating it).

This makes sense to me

marcaaron · 2023-09-01T21:31:30Z


+            if (keysToRemove.length > 0) {
+                _.each(keysToRemove, key => cache.drop(key));
+                promises.push(Storage.removeItems(keysToRemove));


Why not use Onyx.remove() here like we did in Onyx.merge()?

Just because we're bulk-removing items with Storage.removeItems instead of multiple calls to Storage.removeItem

marcaaron · 2023-09-01T21:34:29Z

-            const keyValuePairsForNewCollection = prepareKeyValuePairsForStorage(newCollection);
+                if (persistedKeys.includes(key)) {
+                    existingKeysToKeep.push(key);
+                    keyValuePairsForExistingCollection.push([key, removeNullObjectValues(value)]);


Hmm using removeNullObjectValues() seems like a bug here. Wouldn't that mean that when we merge an object and properties that are set to null (presumably to remove those top-level values) they will keep existing?

Different comment, same line - can we just have existingKeysToKeep.push(key); in this if, and then use the keyValuePairsForExistingCollection.push on line 1280, instead of doubling it here?

Assuming this is still what we want to do, after @marcaaron's comment

dangrous

some additional comments outside of the main discussions

dangrous · 2023-09-05T16:06:40Z

 * @param {Function} [canUpdateSubscriber] only subscribers that pass this truth test will be updated
 */
-function keyChanged(key, data, canUpdateSubscriber) {
+function keyChanged(key, data, canUpdateSubscriber = () => true) {


I'd lean towards this new version as well, as I agree it's more understandable at a glance!

dangrous · 2023-09-05T16:07:41Z

 */
 function applyMerge(existingValue, changes) {
    const lastChange = _.last(changes);
+    if (_.isNull(lastChange)) {


Can you combine this with the if() on new line 1029? Has the same result

dangrous · 2023-09-05T16:09:36Z

    }

-    if (_.isObject(existingValue) || _.every(changes, _.isObject)) {
+    const changesToMake = _.filter(changes, change => Boolean(change));


NAB - might be good to leave a comment here explaining the Boolean thing (i.e. I think I understand this - that it's filtering out any null/undefineds? - but I'm not 100% sure so others might not be either)

dangrous · 2023-09-05T16:12:26Z

+            _.each(collection, (value, key) => {
+                if (value === null) {
+                    keysToRemove.push(key);
+                    delete collectionWithoutKeysToRemove[key];


This makes sense to me

dangrous · 2023-09-05T16:14:41Z

-            const keyValuePairsForNewCollection = prepareKeyValuePairsForStorage(newCollection);
+                if (persistedKeys.includes(key)) {
+                    existingKeysToKeep.push(key);
+                    keyValuePairsForExistingCollection.push([key, removeNullObjectValues(value)]);


Different comment, same line - can we just have existingKeysToKeep.push(key); in this if, and then use the keyValuePairsForExistingCollection.push on line 1280, instead of doubling it here?

Assuming this is still what we want to do, after @marcaaron's comment

roryabraham · 2023-09-05T21:53:26Z

Thanks for the reviews here everyone. While I think this is a valuable improvement, as far as I know there are no active bugs associated with the current behavior. So I'm going to reprioritize a bit and probably not pick this PR back up until next week. Thanks for understanding 🙇🏼

marcaaron · 2023-11-28T02:23:14Z

@roryabraham still focusing on this one? or should we close it and reopen later?

roryabraham · 2023-11-30T01:24:47Z

Haven't focused on it for a bit, but I think it could be a good one to pick back up. Maybe just a holistic look at how deleting stuff from Onyx works today and how we expect everything to work would be a good start

experiment: trying to see if I can get this to fall off my K2

tgolen · 2024-03-28T14:36:05Z

I'm placing this on HOLD right now in order for QA to be done on the latest version of Onyx without more un-QAed PRs from being merged.

roryabraham · 2024-04-22T22:07:32Z

Closing this as not planned for now. I believe there are separate efforts to make merging / setting null more consistent

roryabraham added 2 commits August 21, 2023 22:37

Create failing test

c2bbf65

Dont include null values in waitForCollectionCallbacks

971ce78

roryabraham requested a review from marcaaron August 22, 2023 05:48

roryabraham requested a review from a team as a code owner August 22, 2023 05:48

roryabraham self-assigned this Aug 22, 2023

roryabraham mentioned this pull request Aug 22, 2023

Onyx collection values sometimes appear as null to subscribers. #310

Closed

melvin-bot Bot requested review from dangrous and removed request for a team August 22, 2023 05:48

Pujan92 reviewed Aug 22, 2023

View reviewed changes

marcaaron requested changes Aug 22, 2023

View reviewed changes

roryabraham mentioned this pull request Sep 1, 2023

[HOLD for payment 2023-08-28] [$1000] Offline mode: Crash after deleting Split Bill request Expensify/App#25481

Closed

2 of 6 tasks

roryabraham added 6 commits September 1, 2023 10:05

Merge branch 'main' into Rory-CollectionCallbackIncludesNull

7ca5721

Update set test to include callback

dd310e7

Add test for merge

3c26467

Make merge delete null values like set, don't do null callback to Ony…

f910faf

…x.connect when data is missing

Implement fix in mergeCollection

9b471fd

Fix lint

3226d3c

roryabraham commented Sep 1, 2023

View reviewed changes

Comment thread .eslintrc.js Resolved

roryabraham requested review from Pujan92 and marcaaron September 1, 2023 14:52

marcaaron previously requested changes Sep 1, 2023

View reviewed changes

roryabraham force-pushed the Rory-CollectionCallbackIncludesNull branch from 0f9f59e to 3226d3c Compare September 4, 2023 11:13

Fix applyMerge

9db4bb7

dangrous suggested changes Sep 5, 2023

View reviewed changes

tgolen changed the title ~~Don't include null in collection callbacks~~ [HOLD] Don't include null in collection callbacks Mar 28, 2024

roryabraham closed this Apr 22, 2024

Conversation

roryabraham commented Aug 22, 2023

Details

Related Issues

Automated Tests

Linked PRs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dangrous commented Aug 24, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roryabraham Sep 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dangrous left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcaaron Aug 22, 2023 •

edited

Loading

marcaaron Aug 22, 2023 •

edited

Loading

roryabraham Sep 4, 2023 •

edited

Loading