Resync data when the app reconnects #275

tgolen · 2020-08-20T21:56:30Z

Fixes #253

Tests

Load the app
Go offline (either turn off wifi or set the browser to offline mode)
Post a comment
In another browser, go to expensify.com and go to the same report that is open for the chat just commented on
Post a comment on the report page
Now switch back to the chat app and turn it back to online mode
Verify that you see the comment from 5 show up

tgolen · 2020-08-20T21:57:54Z

Oh, I also renamed the action files. Something @iwiznia requested. Sorry to pollute the PR :D

tgolen · 2020-08-20T21:58:58Z

src/lib/Network.js

-        // Two things will bring the app online again...
-        // 1. Pusher reconnecting (see registerSocketEventCallback at the top of this file)
-        // 2. Getting a 200 response back from the API (happens right below)
+    Ion.get(IONKEYS.NETWORK, 'isOffline')


This change actually removes the need to rely on a local variable for knowing if the app is offline or not

tgolen · 2020-08-20T21:59:19Z

src/lib/Network.js

-        return;
-    }
+                // Make a simple request every second to see if the API is online again
+                fetch(`${CONFIG.EXPENSIFY.API_ROOT}command=Get`, {


This is where I changed request() to fetch()

Can we use request or xhr instead? They have logic like catch that this doesn't

I can try it again, yes. That catch() logic doesn't actually do anything to help what we're trying to do here (ie. we KNOW this is going to fail until the API is back online).

tgolen · 2020-08-20T22:00:49Z

@marcaaron I'm adding you here to take a look at this as well

marcaaron

I just thought of something and I'd like to get your thoughts before we commit to this solution. We don't really need to add a "connect" listener to Network or an extra callback property to the Ion mapping because:

There is only one situation where we must refetch keys (isOffline going from false > true)
Ion already knows when we are going online or offline because it is responsible for watching these changes.
We should just tell Ion which actions it needs to fetch and have Network only worry about updating isOffline.

cead22 · 2020-08-21T00:21:35Z

src/lib/Network.js


-            // If the request failed, we need to put the request object back into the queue as long as there is no
-            // doNotRetry option set in the data
-            if (data.doNotRetry !== true) {


We should still keep this right?

Well, my logic was that there was only one thing using it (the processWriteQueue method), and if that method was no longer going to call request() then it didn't need to stay here. However, if we really want to avoid using fetch() in processWriteQueue then this will probably come back.

That makes sense, but does that mean that requests that go through the network request queue won't be re-tried?

cead22 · 2020-08-21T00:24:15Z

src/page/HomePage/Report/ReportHistoryItem.js

            || nextProps.displayAsGroup !== this.props.displayAsGroup
-            || nextProps.authToken !== this.props.authToken;
+            || nextProps.authToken !== this.props.authToken
+            || !_.isEqual(nextProps.historyItem, this.props.historyItem);


Does this line makes nextProps.historyItem.sequenceNumber !== this.props.historyItem.sequenceNumber redundant?

Ah, yes it does 👍

cead22 · 2020-08-21T00:27:35Z

src/lib/Ion.js

 * @param {string} mapping.statePropertyName the name of the property in the state to connect the data to
 * @param {boolean} [mapping.addAsCollection] rather than setting a single state value, this will add things to an array
 * @param {string} [mapping.collectionID] the name of the ID property to use for the collection
+ * @param {object} [mapping.callback] An alternative to using mapping.reactComponent so that a method can be bound to


An alternative to using mapping.reactComponent , what does this mean? I don't see it anymore

Maybe reactComponent is withIonInstance now?

Oh yep, it was renamed.

cead22 · 2020-08-21T00:27:50Z

src/lib/Ion.js

-                    [mappedComponent.statePropertyName]: newValue,
-                });
+            // If there is a callback attached to the mapping, then trigger the callback and pass it the new value
+            if (mappedComponent.callback) {


if (_.isFunction(mappedComponent.callback)) {

cead22 · 2020-08-21T00:29:47Z

src/lib/Network.js

-        return;
-    }
+                // Make a simple request every second to see if the API is online again
+                fetch(`${CONFIG.EXPENSIFY.API_ROOT}command=Get`, {


Can we use request or xhr instead? They have logic like catch that this doesn't

tgolen · 2020-08-21T01:01:21Z

All right, updated!

marcaaron

LGTM but I can't actually get these test steps to work because when I go offline in Chrome Dev Tools web sockets still work 🤣

marcaaron · 2020-08-21T01:16:45Z

src/lib/Ion.js

-                    [mappedComponent.statePropertyName]: newValue,
-                });
+            // If there is a withIonInstance, then the state needs to be set on that instance with the new value
+            if (mappedComponent.withIonInstance) {


Are we leaving this in case we want to call this without a withIonInstance?

I can remove this and just keep it as a required param for now.

marcaaron · 2020-08-21T01:22:42Z

src/lib/Network.js

+        _.each(reconnectionCallbacks, cb => cb());
+    }
+    isAppOffline = isCurrentlyOffline;
+}


Can we check the store value instead? If I land on this file for the first time it's not clear whether I should use the local variable or Ion to get the offline status. I think we can remove the local variable entirely now.

My recommendation would be to change this to...

async function setNewOfflineStatus(isCurrentlyOffline) { const previousOfflineStatus = await Ion.get(IONKEYS.NETWORK, 'isOffline'); Ion.merge(IONKEYS.NETWORK, {isOffline: isCurrentlyOffline}); if (previousOfflineStatus && !isCurrentlyOffline) { _.each(reconnectionCallbacks, cb => cb()); } }

Ah, OK. That makes sense, yeah. That will help clean this up a little.

... and I see what you did there ...

marcaaron · 2020-08-21T01:52:54Z

Hmm tried with ngrok and it still doesn't seem to be working

marcaaron · 2020-08-21T01:57:26Z

Load the app

Go offline (either turn off wifi or set the browser to offline mode)

Post a comment

In another browser, go to expensify.com and go to the same report that is open for the chat just commented on

Post a comment on the report page

Now switch back to the chat app and turn it back to online mode

Verify that you see the comment from 5 show up

No comment shows up until a page refresh and the comment that was queued to send on the report that went offline never shows up again.

tgolen · 2020-08-21T15:18:16Z

You should still be able to test this with just the offline browser setting. In step 3, when you try to post the comment in offline mode, the failed API call will trigger the isOffline: true state in the app (so you don't have to worry about pusher).

Here is a video of me testing it:

2020-08-21_09-15-37.mp4.zip

tgolen · 2020-08-21T15:19:10Z

Updated

marcaaron · 2020-08-21T16:05:40Z

Alright lemme give it another try.

marcaaron · 2020-08-21T16:15:40Z

I watched your video and noticed that the comment you are leaving on the online tab appears on the offline tab before you set it to online. It is being handled by Pusher somehow and replaces the optimistic comment. Once you come back online everything does seem to work.

I tested it again and got the same result so happy to move forward on this. Especially since I don't think it's possible for an offline comment to get overwritten by Pusher in production.

marcaaron

Looks great to me!

tgolen · 2020-08-21T16:50:13Z

Ah, cool. Glad you got it to work!

tgolen · 2020-08-21T17:23:31Z

@cead22 All you!

cead22 · 2020-08-24T15:57:17Z

src/lib/Network.js

+ */
+function setNewOfflineStatus(isCurrentlyOffline) {
+    Ion.get(IONKEYS.NETWORK, 'isOffline')
+        .then((prevWasOffline) => {


NAB but suggestions for better var names: wasPreviouslyOffline, wasOffline or previouslyWasOffline

marcaaron · 2020-08-24T16:03:45Z

src/lib/Network.js

+            }
+            for (let i = 0; i < networkRequestQueue.length; i++) {
+                // Take the request object out of the queue and make the request
+                const queuedRequest = networkRequestQueue.shift();


fyi and @cead22 looking at this again, I think this is a bug since we are modifying the size of the queue on each iteration?

e.g.

iteration 1: i = 0; i < size 3
iteration 2: i = 1; i < size 2
iteration 3: i = 2; i < size 1

We could do this instead

networkRequestQueue.forEach(queuedRequest => { request(queuedRequest.command, queuedRequest.data) .then(queuedRequest.callback); }); networkRequestQueue = [];

Good catch. We need to make sure that if one request fails that we pause, and then resume from the same spot

Should we make them synchronous then?

Oops, no I don't think so, but to rephrase, we need to make sure we don't lose any of these requests in the process of queueing them / firing them off / ignoring them when offline or reauthenticating

I think it must be. Otherwise I am confused about how this can happen...

Would it be a problem to fetch all at once and then set to an empty array? If we don't care about the order then when any fail then we can re-queue them?

I'm a little confused about what that console code is showing or what the possible bug here is. Can you restate what the bug is that you think is happening?

I don't think we should assume that they can happen in any order, and we should try to preserve the order that they were queued in when possible.

Problem: Given an array of queued network requests with a size greater than 1 we will never process the entire queue in one call of this method with this existing logic.

I don't know if this has any super obvious bug-like effects just yet. But it seems to force the requests that did not get processed on one call to be processed in the next call of this function which happens once per second? The queue will eventually empty. But is that what we want?

Mostly, I'm pointing out that this logic is tricky, doesn't do what it looks like it would do, and doesn't do what we initially wanted... which is to empty the entire queue.

Let me know if that makes sense!

I don't think we should assume that they can happen in any order, and we should try to preserve the order that they were queued in when possible.

To clarify, are you suggesting that the requests should not complete in any order? Or that they need only be requested in a certain order?

Ah, OK. That makes sense, and you're right. Though I don't think it's causing a bug, it is delaying requests unnecessarily and the code isn't very clear. I think using a do/while is probably better. Like this:

const array = [1,2,3]; let i = 0; do { const item = array.shift(); console.log(i); i++; } while (array.length > 0)

To clarify, are you suggesting that the requests should not complete in any order? Or that they need only be requested in a certain order?

Only that they be requested in a certain order.

Cool! In that case, yes this will work. Tbh not too sure I see any clear advantage to using a do/while vs. a for loop or forEach, but maybe you are seeing something I'm not.

tgolen added 9 commits August 20, 2020 13:44

rename action files to not have redundant names

519238c

Add a new option to connect to ion without a react component

7d077c5

Merge branch 'master' into tgolen-resync-data

42261c0

Add an Ion connection to personal details

038971d

Merge branch 'master' into tgolen-resync-data

cab20fd

Remove local variable for isAppOffline

36fce7a

Fix react warning

c47798e

Fetch report history when app comes back online

9e2294b

Merge branch 'master' into tgolen-resync-data

40e2aa5

tgolen requested a review from cead22 August 20, 2020 21:56

tgolen self-assigned this Aug 20, 2020

Lint

b18ffb6

tgolen commented Aug 20, 2020

View reviewed changes

tgolen requested a review from marcaaron August 20, 2020 22:00

marcaaron requested changes Aug 20, 2020

View reviewed changes

tgolen added 2 commits August 20, 2020 16:38

Improve comment

5a80816

Don't connect to Ion from network

1aa951f

cead22 requested changes Aug 21, 2020

View reviewed changes

tgolen added 4 commits August 20, 2020 18:57

Merge branch 'master' into tgolen-resync-data

16e9af0

Switch to using xhr instead of fetch

bf4658c

remove some ion changes that don't need to be there

45e77c5

Remove redundant logic

7a3cd9c

marcaaron previously approved these changes Aug 21, 2020

View reviewed changes

Remove local variable and make withIonInstance required

a148c31

tgolen dismissed marcaaron’s stale review via a148c31 August 21, 2020 15:19

marcaaron approved these changes Aug 21, 2020

View reviewed changes

cead22 approved these changes Aug 24, 2020

View reviewed changes

cead22 merged commit fb03397 into master Aug 24, 2020

cead22 deleted the tgolen-resync-data branch August 24, 2020 15:59

marcaaron reviewed Aug 24, 2020

View reviewed changes

Resync data when the app reconnects #275

Resync data when the app reconnects #275

Uh oh!

Conversation

tgolen commented Aug 20, 2020

Tests

Uh oh!

tgolen commented Aug 20, 2020

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!

tgolen commented Aug 20, 2020

Uh oh!

marcaaron 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!

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!

tgolen commented Aug 21, 2020

Uh oh!

marcaaron 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!

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 commented Aug 21, 2020

Uh oh!

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

Uh oh!

Uh oh!

tgolen commented Aug 21, 2020

Uh oh!

tgolen commented Aug 21, 2020

Uh oh!

marcaaron commented Aug 21, 2020

Uh oh!

marcaaron commented Aug 21, 2020

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

tgolen commented Aug 21, 2020

Uh oh!

tgolen commented Aug 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

marcaaron commented Aug 21, 2020 •

edited

Loading

marcaaron Aug 24, 2020 •

edited

Loading

cead22 Aug 24, 2020 •

edited

Loading