Quit early when persistedRequests is empty #3511
Conversation
src/libs/Network.js
Outdated
| @@ -32,7 +32,7 @@ let didLoadPersistedRequests; | |||
| Onyx.connect({ | |||
| key: ONYXKEYS.NETWORK_REQUEST_QUEUE, | |||
| callback: (persistedRequests) => { | |||
| if (didLoadPersistedRequests || !persistedRequests) { | |||
| if (didLoadPersistedRequests || !persistedRequests || persistedRequests.length === 0) { | |||
There was a problem hiding this comment.
Is this correct? Based on the issue I just read it seems like didLoadPersistedRequests should be preventing this callback from executing - but does not because we are calling Onyx.set() before updating that variable. LMK if I've got it wrong. Probably we should...
- Move the
didLoadPersistedRequestsabove theOnyx.set()in this function to solve the issue for now - Fix Onyx so that
keysChanged()is async (what we have before) to prevent other potential issues where we have assumed an asynchronous callback
There was a problem hiding this comment.
Yes that solution also works, but is there a reason why we would want to continue running if have an empty array?
There was a problem hiding this comment.
I'm still happy to switch to just do the other change though. That way we'll have close to original behavior as possible.
There was a problem hiding this comment.
is there a reason why we would want to continue running if have an empty array
No, I can't really think of one. But it's not the issue we are trying to fix here and doesn't seem to be causing any specific problem for us.
|
Updated and tested it works on iOS and the others I've checked off on the list 👍🏾 |
|
🚀 Deployed to staging in version: 1.0.66-13🚀
|
|
🚀 Deployed to production in version: 1.0.68-4🚀
|
@Dal-Papa, please review when you get the chance
CC: @marcaaron, in case I'm misunderstanding how this is supposed to work.
Details
This callback was looping too quickly because of this line (which recursively calls the callback function it's in): https://github.com/Expensify/Expensify.cash/blob/afb44ff4667bb5686c81c074097af60c26913a5c/src/libs/Network.js#L42
The actual implementation of this function doesn't change. If
persistedRequestsis empty,networkRequestQueue = [...networkRequestQueue, ...persistedRequests];becomesnetworkRequestQueue = [...networkRequestQueue, ...[]];which essentially does nothing over and over again.My theory for why this hasn't happened before is that Onyx wasn't fast enough to finish the
Onyx.set(..., [])before thedidLoadPersistedRequests = trueline right after it. If it was slow, that would be set to true and this would exit early as expected. Alternatively we could swap line 42 and 43, but I think this change makes more sense.Fixed Issues
Fixes #3485
Tests
Did QA on each of the platforms (couldn't get android to work with the offline mode though I can't do that with production either, so I think that's due to my VM)
QA Steps
In each respective platform (specifically iOS is broken),
Tested On
Screenshots