Try to synchronize operations within one update

[paultsimura]

Details

This PR changes how we process the Onyx.update operation:

1. Enqueue all the operations per individual key;
2. Group the collection-related keys into a single mergeCollection call;
3. Process the rest of the individual keys with merge calls;

Related Issues

https://github.com/Expensify/App/issues/37560

Automated Tests

I've added tests that cover the Onyx.update() with multiple merge operations of the same keys to verify that all the changes are batched into a single mergeCollection operation.

Manual Tests

[!WARNING]
Since this change alters one of the most common Onyx operations – we should perform a general regression testing of the system.

1. Perform a complex Onyx.update() operation with multiple updates of the same item in merge and mergeCollection operations.
2. Verify the collection items are updated with all the changes combined.

Operation example ref: https://github.com/Expensify/react-native-onyx/blob/eb17a5dd0eba6b7b27f1e92e0fc81863091ba6c8/tests/unit/onyxTest.js#L1046-L1103

Author Checklist

• [x] I linked the correct issue in the ### Related Issues section above
• [ ] I wrote clear testing steps that cover the changes made in this PR
  • [ ] I added steps for local testing in the Tests section
  • [ ] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
• [ ] I included screenshots or videos for tests on all platforms
• [ ] I ran the tests on all platforms & verified they passed on:
  • [ ] Android / native
  • [ ] Android / Chrome
  • [ ] iOS / native
  • [ ] iOS / Safari
  • [ ] MacOS / Chrome / Safari
  • [ ] MacOS / Desktop
• [ ] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
• [ ] I followed proper code patterns (see Reviewing the code)
  • [ ] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
  • [ ] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
  • [ ] I verified that comments were added to code that is not self explanatory
  • [ ] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
  • [ ] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
  • [ ] I verified the JSDocs style guidelines (in STYLE.md) were followed
• [ ] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
• [ ] I followed the guidelines as stated in the Review Guidelines
• [ ] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
• [ ] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
• [ ] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
• [ ] I verified that if a function's arguments changed that all usages have also been updated correctly
• [ ] If a new component is created I verified that:
  • [ ] A similar component doesn't exist in the codebase
  • [ ] All props are defined accurately and each prop has a /** comment above it */
  • [ ] The file is named correctly
  • [ ] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
  • [ ] The only data being stored in the state is data necessary for rendering and nothing else
  • [ ] If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
  • [ ] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
  • [ ] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
  • [ ] All JSX used for rendering exists in the render method
  • [ ] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
• [ ] If any new file was added I verified that:
  • [ ] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
• [ ] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
• [ ] If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
• [ ] I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

[github-actions[bot]]

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

[paultsimura]

@tgolen I think I found a way to fix the issue of the out-of-order updates.

The main idea is the following: when an update call comes with multiple merge/set/mergeCollection operations, – do not process anything asynchronously. Instead, treat it as one transaction: stack all the operations by onject keys in the same exact order they come in the update data, calculate the diff in-memory, and then persist everything in one multiGet + multiSet take.

This PR is super draft, it lacks the validations as well as the broadcasting of the updates, but I would appreciate your opinion on the idea as a whole.

[paultsimura]

In general, it does work – I gave it a little test run. However, there are a lot of different callbacks in the current implementation and I'd appreciate your guidance on a more general level @tgolen.

Calls like these:

https://github.com/Expensify/react-native-onyx/blob/7cbf836cc1c107e82da42f372b82c91e5ecd1640/lib/Onyx.js#L1511-L1516

https://github.com/Expensify/react-native-onyx/blob/7cbf836cc1c107e82da42f372b82c91e5ecd1640/lib/Onyx.js#L1153-L1154

And all the places where we call the sendActionToDevTools. With the new approach, if we treat update as a single transaction with the eventual merge, it will be much harder to differentiate mergeCollection from merge.

What broadcasting approach would you suggest using here? If I broadcast each key update separately – would the collection listener handle each update individually? Should I instead broadcast all collection-related keys as a single collection update, and non-collection keys as individual updates?

[tgolen]

If I broadcast each key update separately – would the collection listener handle each update individually?

I believe this would work yes, and the collection listener will handle each individual update. This would be the safest approach against causing regressions.

Should I instead broadcast all collection-related keys as a single collection update, and non-collection keys as individual updates?

The collection updates get a little tricky because some collection mappings will be expecting the callback to be triggered with the entire collection, while others might be expecting the callback to be triggered with each object in the collection. This is dependent on the waitForCollectionCallback mapping option.

I think I prefer the first method because that will make the least amount of behavior changes for Onyx.

[paultsimura]

This looks like a promising picture to me: when coming online, 46 operations were batched into one update:

Some of the keys had up to 7 merge operations stacked:

But eventually, the broadcast was called for each key only once, containing the final updated version of the object:

[paultsimura]

I have read the CLA Document and I hereby sign the CLA

[paultsimura]

@tgolen please make a sanity check when you have a minute, along with this comment.

The only thing that bothers me is that one failing test. I don't fully understand the nature of those subscriptions, and based on the comment in code, that first call is expected to be with null, undefined params 🤔 https://github.com/Expensify/react-native-onyx/blob/7cbf836cc1c107e82da42f372b82c91e5ecd1640/lib/Onyx.js#L915-L918

[tgolen]

Wow, that is a great improvement!

I don't think the test is too much to worry about. You can update the tests to change the nth to be 0 instead of 1 now. It makes sense in the context of the change you're making.

Before your PR:

• This Onyx.update() would have triggered each subscriber twice (first with null, second with the actual value)

After your PR:

• The updates are squished into a single callback with the final value 🎉

I think the important part of null that needs to be kept in mind (and you might want to add this to the unit tests with these changes) is that if I subscribe to a key that doesn't have any data stored on it, then the subscriber should be triggered once with null for a value. This allows the component to render and then the null value is substituted for undefined in withOnyx here. That way, when the component gets the undefined prop, the defaultProp will be picked up and used.

Does that all make sense?

[paultsimura]

Does that all make sense?

Yes, updated the test as you recommended – to verify that the first call for the non-existing keys is made with undefined.

[paultsimura]

@tgolen how terrible is the idea to make the following change?

case METHOD.MERGE_COLLECTION: 
    _.each(value, (_value, _key) => promises.push(() => merge(_key, _value)));
    break; 

https://github.com/Expensify/react-native-onyx/blob/69b2d5973b4aef19ce783440df55dedb30d4efd2/lib/Onyx.js#L1560-L1562

From what I understand, it will trigger the re-render of components listening to a collection N times?

[tgolen]

We use mergecollection quite a bit so I would definitely be worried about performance issues with that.

[paultsimura]

@tgolen I think I've made a working solution: it updates the data in the correct order, but it's a bit slower than the current version.

Would it be possible to request a look from the side at the potential bottlenecks and improvements, especially the cache-related operations? This PR is risky because it changes one of the mostly used Onyx functions, so an extra pair of eyes would really be helpful here.

[tgolen]

OK, great. Let me ping some of our experts on Slack to see if they can have a look at this PR. You can also see my comment here about how it can be reliably tested to help protect against major regressions.

[paultsimura]

@kidroca could you please help me understand whether the mergeQueue was introduced to chain the merge operations within one update call, or if there's more to it? How broad is the "single tick" definition?

https://github.com/Expensify/react-native-onyx/blob/3f77c6bd2030979da3d581ea01fc9fd5e6d646e5/lib/Onyx.js#L23-L25

https://github.com/Expensify/react-native-onyx/blob/3f77c6bd2030979da3d581ea01fc9fd5e6d646e5/lib/Onyx.js#L1285-L1291

[paultsimura]

I just discovered a behavior that looks like a blocker to me. Explained in detail here: https://github.com/Expensify/react-native-onyx/issues/301#issuecomment-1987950312

[kidroca]

@kidroca could you please help me understand whether the mergeQueue was introduced to chain the merge operations within one update call, or if there's more to it? How broad is the "single tick" definition?

The merge queue was created way before the update interface existed, AFAIK it serves use cases where Onyx.merge is called multiple times from actions in the same tick, to be fair I don't know exactly when it succeeds in batching more than one item to merge, I guess it would be during init or updates

[paultsimura]

I think we should ask for a review from @jjcoffee since he's a C+ on the related issue. Also, we need a full regression test of the App with the version bumped.

[paultsimura]

@tgolen it would be great to get your 👀 on this PR before requesting the full regression test.

[paultsimura]

While testing the PR deeper, I've noticed another interesting bug: The mergeCollection calls each item subscriber every time, even if the data did not actually change – we just fan out the latest cached value of the item to its subscriber (this is different in the merge operation – we call the subscriber's callback only if the item actually changed).

The issue with this is that withOnyx returns a new object each time, even though it's equal to the previous one. As a result, we have endless hook loops where an Onyx object is a dependency, e.g. here:

https://github.com/Expensify/App/blob/22bd9eb633f0d378158bc3b8674682fbc39a56ea/src/pages/iou/request/step/IOURequestStepConfirmation.js#L129-L134

WDYR says this:

different objects that are equal by value.

@kidroca because of this, I'm returning back the merge operation for 1-item collections. @tgolen this looks like a pretty unpredictable hole to me since the mergeCollection basically makes the object-dependencies in hooks useless, and we use such dependencies all across the App. WDYT?

[paultsimura]

Found and reported another bad mergeCollection issue: https://github.com/Expensify/react-native-onyx/issues/514

[tgolen]

@tgolen this looks like a pretty unpredictable hole to me since the mergeCollection basically makes the object-dependencies in hooks useless, and we use such dependencies all across the App.

I agree, that is a bad hole. Did you consider changing mergeCollection() so that subscribers are only triggered if the object changes (so it works like merge())? I think that should be a reasonable change to make. I think it would also be fine to do that in a follow-up issue so that PR you are working on here doesn't have the scope increased.

I'm just getting caught up from being on vacation last week. Is there anything that I can help with for a next step here? How is the QA coming on the original PR and what is the status/ETA of getting this merged?

[paultsimura]

How is the QA coming on the original PR and what is the status/ETA of getting this merged?

This PR unfortunately must be held for the other Onyx issues I've reported along the way – I'll retest once those are done and update here.

[tgolen]

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.

[paultsimura]

@blazejkustra is there any chance you could help me with reviewing this TS-wise? 🙏 There are a couple of tough places I'm not entirely sure how to implement since the TS migration.

[blazejkustra]

@paultsimura I should be able to help with this! 😄

OnyxUtils.js migration is close to a merge (@fabioh8010 is testing it in NewDot).

Would it be possible to wait with this PR until the migration is merged? This could resolve some of your type problems + you won't have to care about declaration OnyxUtils.d.ts file.

[paultsimura]

Would it be possible to wait with this PR until the migration is merged?

Absolutely 👍

[blazejkustra]

Update: OnyxUtils migration is in it's final state, waiting to be merged 😄

[blazejkustra]

@paultsimura Update: OnyxUtils was merged yesterday, we can remove HOLD from the title!

[paultsimura]

Update: OnyxUtils was merged yesterday, we can remove HOLD from the title!

Thanks @blazejkustra. That PR caused one blocking regression, reported here: https://github.com/Expensify/react-native-onyx/pull/523/files#r1568401082

[blazejkustra]

@paultsimura Please have a look at the fix here. Thanks for reporting!