'Burn with Fire' is too extreme

[designmanio]

'Burn with Fire' is too extreme

[adamgins]

@JulianKingman just to decipher the above diagram...

basically, I think when we're online we're all good now - ie delete works. The "bur with fire" approach fixed the #29

The issue, it fired every time you did something like a find ... ie !this.offline && which means that if you were sitting one page and did a find, all the other docs would have been deleted too.

So as @Diwei-Chen say we only really need to "burn with fire" when a user comes back from being offline (not every time). Does that make sense?

Potentially, you could also check gow long it's been offline for... ie if it only lost connection for a few seconds, you probably don't want to burn with fire, thoughts?

[JulianKingman]

I'm pretty sure it doesn't burn with fire anymore, just removes the difference. Here's what's supposed to happen, step-by-step:
(edited, had the order of steps 1 & 2 wrong)

1. User is online, DDP 'added' documents are cached in redux, their ID is cached in reactNativeMeteorOfflineRecentlyAdded
2. Subscription is ready, MO compares reactNativeMeteorOfflineRecentlyAdded with cached documents, there are no cached documents, so REMOVE_AFTER_RECONNECT fires removing 0 docs, and clearing reactNativeMeteorOfflineRecentlyAdded.
3. User goes offline, cached data is used. If the user opens the app and it's offline, minimongo populates from persisted redux store.
4. User comes online again. When the subscription is ready, collection checks reactNativeMeteorOfflineRecentlyAdded to see if cached versions exist that no longer exist online, and removes those from redux. I may be missing a step where it also removes it from minimongo, is that the case?

[JulianKingman]

I see a problem here, this.offline isn't reactive, so I'll use Meteor.status() instead. Give me a minute to fix.

[JulianKingman]

OK, I made some progress here: 39d0070

The problem now is that ready is calling before all documents are loaded, and then the redux store gets dumped. I'll continue this tomorrow, but in the meantime if you can see what needs doing, let me know or make a pr :)

[JulianKingman]

(It also removes from minimongo now, so the issue is more obvious; before it was hiding)

[adamgins]

@JulianKingman thanks. The diff check is awesome.

So am I understand correctly I could define a debounce option of let's say 30 seconds, which would deal with real life scenarios like split second loss of connection?

The issue is I don't think it's a pure debounce... ie you don't even want to run it once in those situations... almost needs a timeout before it starts checking or time since it last ran? ie debounce will run once, then prevent it from running again, within the time limit... we really want it to ignore small loss of connections... eg let's say 30 seconds (some user defined option).

Thinking of the scenarios of driving down a windy road where mobile coverage comes in/out and the app would be trying to do this... perhaps this is not an issue assuming the diff code works???

Just thinking out loud

[Diwei-Chen]

Hi @JulianKingman Thanks for explanations and the updates!

As you said, in step 1, DDP 'added' documents have been cached in redux, so for step 2, shouldn't it be the keys of getState()[collection] are the same as reactNativeMeteorOfflineRecentlyAdded[collection] so _.difference(cached, added) returns []?, which means the cached docs are the same as the recently added docs then we remove 0 docs?

[Diwei-Chen]

I think calling collection(collection, subscriptionName) multiple times or even second time will cause problems.

Here is the case:

Let's say we have three resources which are 1, 2, 3,

for the first time we call the collection function after we subscribe, we will have

added = [1, 2, 3],
cached = [1, 2, 3],
removed = [],

Everything is fine here, and after that reactNativeMeteorOfflineRecentlyAdded becomes blank by doing newState.reactNativeMeteorOfflineRecentlyAdded[collection] = [] in REMOVE_AFTER_RECONNECT

for the second time, it will become

added = [],
cached = [1, 2, 3],
removed = [1, 2, 3]

At this point of time, all the resources in cached will be removed.

So I was thinking we only call newState.reactNativeMeteorOfflineRecentlyAdded[collection] = [] only when we are reconnecting to the internet?

[Diwei-Chen]

Hi @JulianKingman , I just made a pr and it seems like fixing the issue for me. The pr only does newState.reactNativeMeteorOfflineRecentlyAdded[collection] = [] when we are offline, then as long as we are back to online, it will compare the difference.

regards,

[JulianKingman]

Thanks! There were some other issues as well, but I implemented the offline check. Other things implemented:

• Checks if it's the first time running since coming online (otherwise when a subscription is "ready" it will run again and problems ensue)
• Fixed callback for offline use (only runs once now)
• Stopped caching reactNativeMeteorOfflineRecentlyAdded (was causing problems)

One thing to note is that the debounce interval is for the persister, not how often it checks. I don't put any limits on the check because it's cheap, and because DDP is going to add everything on reconnect anyway. So what the debounce interval does is allows you to say "only persist the redux state this often". I think 1s is a healthy number.