Investigate which react-native-onyx package caused the deploy blocker

[chiragsalian]

We received this deploy blocked recently. To fix it we reverted the react-native-onyx package here

We need to investigate which PR in that repo caused the problem. I suspect its this one but i haven't confirmed.

Either way, some next steps are,

1. Revert that PR and bump up the react-native-onyx version. Or,
2. Improve that PR and then bump up the react-native-onyx version.

cc @rory, @marcaaron & @Szymon20000 since i see you guys have updated react-native-onyx most recently.

Action Performed:

1. In NewDot pull in version 1.0.24.
2. Run npm i. If react-native-onyx version didn't update in node-modules then delete the folder and rerun npm i.
3. Log into any account. Get its accountID.
4. If its a new account, then message at least two people so that you have some chats on LHN.
5. In psysh run,

use Expensify\Onyx;
use Expensify\OnyxKeys;
Onyx::pushUpdatesToClients([<accountID>], [["onyxMethod" => "mergecollection","key" => "reportIOUs_", "value" => []]]);

7. If you don't have psysh it's a bit harder to simulate. I think you need to go to settings -> workspace -> manage members and add someone you haven't chatted with before.
8. Click into a different tab and come back to newDot's tab.

Expected Result:

1. LHN should continue to look normal.

Actual Result:

1. LHN chats are either empty or show very few of them.

Workaround:

User has to refresh to see their chats.

Platform:

Where is this issue occurring?

• Web
  QA reported it not working on android earlier. I'm unsure and haven't tested the failure elsewhere besides web.

[melvin-bot]

Triggered auto assignment to @slafortune (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

[JmillsExpensify]

@chiragsalian I don't see anyone assigned to this issue, so I wanted to clarify how we're handling next steps?

[marcaaron]

I think we just need to go one version at a time and see which one breaks this behavior? I can help look into it.

[chiragsalian]

I don't see anyone assigned to this issue, so I wanted to clarify how we're handling next steps?

Yup i just cc'd relevant people to discuss first. But yes its basically what marc suggested. Btw marc, before going back one version at a time how about just reverting this one and testing? If its not the issue then it would make sense to revert one version at a time.

[marcaaron]

@chiragsalian I am a little confused, but why would running this code trigger the bug?

Onyx::pushUpdatesToClients([<accountID>], [["onyxMethod" => "mergecollection","key" => "reportIOUs_", "value" => []]]);

[chiragsalian]

So i don't know why it triggers the bug but it did. For some reason when updating reportIOUs_, this callback was triggered and it would update reports which is why the LHN list would become empty.

I can demo the behavior to you as well in a video chat if you'd like.

[marcaaron]

Weird.

[marcaaron]

Narrowed it down to 1.0.19. Which I think is the version where we introduced the fastMerge() method. Will keep digging...

[marcaaron]

@chiragsalian One more thing... am still a little confused about why we would send an empty array as a collection and how we came to this conclusion? A collection passed to mergeCollection() should never be an array.

[marcaaron]

I think you need to go to settings -> workspace -> manage members and add someone you haven't chatted with before.

Click into a different tab and come back to newDot's tab.

I wasn't able to reproduce this so unsure what the next steps here should be.

[marcaaron]

Oh hmm maybe it's this:

https://github.com/Expensify/Web-Expensify/blob/3bd0e890d19509687f7e4854e96b81b824b31c0b/lib/AppInit.php#L120-L138

Going to test with an account that has no IOUs and see if the same happens.

[marcaaron]

Gonna have to dive into the exciting world of updating the VM to test this theory 💀

[chiragsalian]

am still a little confused about why we would send an empty array as a collection and how we came to this conclusion?

So i can only answer how we came to this conclusion while testing. Its because the blocker issue mentioned a problem with adding members. When i reproduced the same on dev i looked at this line. $reportOnyxData contained values to update report and reportIOUs. So i broke them into two. I noticed when it sent a push notification for reports nothing was broken but when it attempted to send the push notification to reportIOUs things broke. As for it being empty. When i was debugging PHP I'm pretty sure $iouReports in fetchReports was empty so that's what Onyx::pushUpdatesToClients sent.

As for testing steps, make sure you are on onyx version 1.0.24, you should also check if your node-modules matches the same. I had to nuke node-modules at one point.

[marcaaron]

Alright so the reproduction is:

1. Have an account with no IOUs
2. Be on a version of react-native-onyx after 1.0.19
3. Invite user to workspace
4. Verify the sidebar gets messed up

I don't actually think there is anything wrong with Onyx though. It's not supported to set an array as a collection so I think we should:

• Fix the usage I found above in PHP (and other usages)
• Start throwing an error if an empty array is provided to the Onyx method in PHP and mergeCollection() is used

[marcaaron]

As for why it doesn't blow up in the version before the fast merge one...

cache.merge() method was doing this before:

    merge(data) {
        this.storageMap = mergeWithCustomized({}, this.storageMap, data);

So in the case where we called this with an empty array instead of an object it would still return the storage map e.g.

However, in the new code if you pass an array to fastMerge() then it will return an array. And if we use Object.assign() on that it will blow the entire cache away 😬

    merge(data) {
        this.storageMap = Object.assign({}, fastMerge(this.storageMap, data));

So, I think we also better update Onyx so that it's not possible to ever call cache.merge() with something that's not an object.

[chiragsalian]

I think we also better update Onyx so that it's not possible to ever call cache.merge() with something that's not an object.

I like this plan.

Fix the usage I found above in PHP (and other usages)

Instead of fixing individual usages how about we update Onyx::pushUpdatesToClients to remove arrays (or convert them to stdClass) if $onyxUpdates "value" field is empty?

[marcaaron]

Instead of fixing individual usages how about we update Onyx::pushUpdatesToClients to remove arrays (or convert them to stdClass) if $onyxUpdates "value" field is empty?

Feel free to hop on the review and chime in there. That crossed my mind, but felt kind of magical to me so I'd be curious to get more opinions on it. I had a feeling like it's important for everyone to understand that empty arrays do not turn into objects magically and that it's up to you to responsibly pass the correct values.

[marcaaron]

Onyx bump PR in review now. Tested and it resolves the issue here related to mergeCollection(). Small thing to keep in mind is that older versions of the app will be affected and an update to a newer version will be necessary to fix any weirdness.