meteorrn / meteor-react-native

Meteor client for React Native matching Meteor Spec
https://guide.meteor.com/react-native.html
Other
59 stars 31 forks source link

Local Collection not reactive in useTracker in core version 2.5.0 #122

Closed bratelefant closed 11 months ago

bratelefant commented 1 year ago

If you use the Local Collection (like new Local.Collection("tasks") instead of new Mongo.Collection("tasks")), useTracker will not be triggered on an "added" or "changed" event.

I recreated this behaviour in a fork of the repo meteor-react-native-starter

bratelefant commented 1 year ago

it broke already at release 2.4.0

jankapunkt commented 1 year ago

Seems to be introduced in https://github.com/meteorrn/meteor-react-native/pull/111 the big question is, by which part of it. This also shows, that there was not enough test coverage until that point, otherwise this regression would have been detected.

@ToyboxZach can you help out with this? Maybe we can fix this one together? I'd like to write some tests, would you mind taking a look at the reproduction repo and your former pull request and find out, what change this actually caused?

bratelefant commented 1 year ago

It can be fixed in the local collection by using LocalCols insert and update methods rather than going for minimongos upsert directly.

Eg by turning this

LiveCol.find({}).observe({
        added: async (doc) => {
          LocalCol._collection.upsert(doc);
          storeLocalCol();
        },
....
    };

into this

LiveCol.find({}).observe({
        added: async (doc) => {
          LocalCol.insert(doc); // this informs all observers and triggers useTracker
          storeLocalCol();
        },
....
    };

I'll propose a PR on this. Fixes the issue. But tests for this seem a good idea to make things future proof.

However, as the local collection hasn't changed in #111 , it seems like the trackers behavior did since it now does no longer catch up with modifications on minimongo.

Question is, whether this is a wanted or unwanted behavior.

jankapunkt commented 1 year ago

In any case we definitely need better test coverage for collection reactivity

bratelefant commented 1 year ago

Interestingly, if you try to add a "removed" observer to the LiveCol and try to also make Local Collection reflect removed docs, reactivity fails on removed docs even when using LocalCol.remove().

jankapunkt commented 1 year ago

@bratelefant

Interestingly, if you try to add a "removed" observer to the LiveCol and try to also make Local Collection reflect removed docs, reactivity fails on removed docs even when using LocalCol.remove().

Do I assume correctly, that #123 would require some additions in order to resolve reactivity with remove?

ToyboxZach commented 1 year ago

Yeah sorry, my use cases doesn't utilize any local collections so I wasn't focused on it, I was dependent on the tests to work so there is a good chance I missed triggering the tracker for a lot of cases of local handling

bratelefant commented 1 year ago

Do I assume correctly, that #123 would require some additions in order to resolve reactivity with remove?

Actually not, that's why I'm surprised.

ToyboxZach commented 1 year ago

I think the problem is the original code was using _collection, which was not setup to utilize the new tracker infrastructure, switching to the normal .insert/.update functions will make it work the same way as the remote collections, so I would expect this to work.

Honestly I'm not quite sure what the purpose of the separate Local.Collection is vs just using the Mongo.Collection without a name, which is how normal meteor mongo creates local collections.

My suggestion would be to remove Local.Collections completely and fix any issues that come up in Mongo.Collection where we aren't pushing the right reactivity

bratelefant commented 1 year ago

I guess nameless Mongo collections only live in memory. Local collections use async storage and get updated automatically as soon as ddp is reconnected. I use this to get my app offline first ready.

However, local collections don't recognize removed documents. Guess this is due to the fact that when coming back, initially docs are added (current state from the pub) but nothing "old" is removed. That's why I suggested something like "stale" local documents, that could be old local documents that don't exist on the server anymore and could be removed from the client after a reconnect.

ToyboxZach commented 1 year ago

Hmm that behavior seem like it should instead be an option on meteorrn/minimongo ,

but IMO the better solution would be for each app to handle that, since what I see in LocalCollection doesn't feel like a general solution.

I think a basic implementation would be:

goToBackground(){
const data = collection._collection.db.serialize()
saveDataToDisk(data)

}
comeFromBackground(){
const data = loadDataFromDisk()
collection._collection.db = MemoryDb.deserialize(data);
}
bratelefant commented 1 year ago

I personally consider "offline first" a quite general feature / approach that you might want in your app. Get data from the server, keep it on device, update asap. Hence, it's legit to have this in a package.

@ToyboxZach s example shows how everyone would struggle with all possible situations; for instance this implementation would not work on a disconnect, or what if you reopen the app without a connection?

bratelefant commented 1 year ago

Ok, maybe you've got a point there @ToyboxZach . Tried to get a conceptual solution for an offline first approach by trying to patch things in Local.Collection and the tracker (cf. my draft PR #124), but that's somewhat hacky.

I now created a "useCachedTracker" wrapper for "useTracker" that handles offline caching quite nicely; if you're offline or the sub is not ready yet, offline data is used, otherwise "live" data. This boost speed since AsyncStorage is faster than web, you always see the latest data even if offline and if you're connected you have full reactivity.

jankapunkt commented 1 year ago

@bratelefant @ToyboxZach can I close this issue or should it be kept open until #124 is complete and merged? will keep this up

jankapunkt commented 1 year ago

@bratelefant @ToyboxZach can we generalize this a bit? From my perspective I see the following:

To me this is an attempt for at least a new minor release. What do you think @bratelefant @ToyboxZach ?

bratelefant commented 1 year ago

IMO the most important thing is to make the behavior really predictable. Right now, based on the current release of useTracker in this package, I made a really huge step towards offline first by creating a useCachedTracker, which, roughly speaking, wraps around useTracker and stores all calculation results in local storage. Also, it listens to connection status and checks if the subs are ready. As long as we're offline or the sub is not ready it fetches data from cache, otherwise "live" data is used and the cache gets updated.

This results in really speedy ui, while preserving full reactivity. But it's crucial, that the "subReady" event and connection status are really on point. I observed that sometimes (especially when bringing up the app from background) docs get removed and added again (although they didn't change serverside), while the sub is still "ready".

jankapunkt commented 1 year ago

IMO the most important thing is to make the behavior really predictable. Totally agree!

Right now, based on the current release of useTracker in this package, I made a really huge step towards offline first by creating a useCachedTracker, which, roughly speaking, wraps around useTracker and stores all calculation results in local storage. Also, it listens to connection status and checks if the subs are ready. As long as we're offline or the sub is not ready it fetches data from cache, otherwise "live" data is used and the cache gets updated.

I think this fits for the above specs. The "strategies" are only an idea, in case we may have conflicting use-cases.

This results in really speedy ui, while preserving full reactivity. But it's crucial, that the "subReady" event and connection status are really on point. I observed that sometimes (especially when bringing up the app from background) docs get removed and added again (although they didn't change serverside), while the sub is still "ready".

I think we need a reproduction repo where we can investigate this. Can you recreate this issue by any chance?

bratelefant commented 1 year ago
  • we have the following use-cases

    • app goes to background
    • app comes from background
    • app is closed / exited
    • app is started

Each of these use-cases can occur while being online or offline; eg. in flight-mode or online but the server is not reachable.

  • I think we should leverage things like a "strategy" pattern to solve the question on how stale docs are handled

    • wipe - just remove all and only add new docs, consider flashing UI with this
    • diff - keep stale docs, load new docs and remove remaining stale once all added
    • keep - keep everything and let the app's implementation logic decide what to remove
    • custom - some entirely custom implementation
  • what else is missing?

That's basically it I guess. Key point is that DDP doesn't send "remove" or "update" events to the client for all changes that occured since the last disconnect, if I got it right... (at least this would be reasonable to do it like that, since there could possibly a lot of stuff to catch up with on the client)

Is there any way to get the exact point from DDP on where the point of "once all added" is reached? This would be the golden path for the diff strategy

EDIT: If the Local.Collection could deal with reactivity on added, changed and even removed events in a tracker, without removing all docs before the initial adds finished, this would be basically it

bratelefant commented 1 year ago

IMO the most important thing is to make the behavior really predictable.

Totally agree!

Right now, based on the current release of useTracker in this package, I made a really huge step towards offline first by creating a useCachedTracker, which, roughly speaking, wraps around useTracker and stores all calculation results in local storage. Also, it listens to connection status and checks if the subs are ready. As long as we're offline or the sub is not ready it fetches data from cache, otherwise "live" data is used and the cache gets updated.

I think this fits for the above specs. The "strategies" are only an idea, in case we may have conflicting use-cases.

This results in really speedy ui, while preserving full reactivity. But it's crucial, that the "subReady" event and connection status are really on point. I observed that sometimes (especially when bringing up the app from background) docs get removed and added again (although they didn't change serverside), while the sub is still "ready".

I think we need a reproduction repo where we can investigate this. Can you recreate this issue by any chance?

You can eg get your meteorrn starter repo or any minimal repo and add a "disconnect" / "reconnect" button and observe the "ready" state of a sub handler, and also eg log "added" events of an arbitrary collection that fetched data via this sub. The sub will constantly be "ready".

Cf #128

github-actions[bot] commented 12 months ago

Closing this issue due to no activity. Feel free to reopen.