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

Quit early when persistedRequests is empty #3511

Merged
merged 2 commits into from
Jun 10, 2021
Merged

Quit early when persistedRequests is empty #3511

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
afb44ff
Quit early when persistedRequests is empty
Jun 10, 2021
56f152a
Keep original logic and move didLoad persisted call
Jun 10, 2021
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 src/libs/Network.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ let didLoadPersistedRequests;
Onyx.connect({
key: ONYXKEYS.NETWORK_REQUEST_QUEUE,
callback: (persistedRequests) => {
if (didLoadPersistedRequests || !persistedRequests) {
if (didLoadPersistedRequests || !persistedRequests || persistedRequests.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

  1. Move the didLoadPersistedRequests above the Onyx.set() in this function to solve the issue for now
  2. Fix Onyx so that keysChanged() is async (what we have before) to prevent other potential issues where we have assumed an asynchronous callback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that solution also works, but is there a reason why we would want to continue running if have an empty array?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm still happy to switch to just do the other change though. That way we'll have close to original behavior as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return;
}

Expand Down