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.

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

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

Closed
bratelefant opened this issue Jul 4, 2023 · 20 comments · Fixed by #123
Closed

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

bratelefant opened this issue Jul 4, 2023 · 20 comments · Fixed by #123
Labels
bug Something isn't working no-issue-activity

Comments

@bratelefant
Copy link
Collaborator

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 bratelefant added the bug Something isn't working label Jul 4, 2023
@bratelefant
Copy link
Collaborator Author

it broke already at release 2.4.0

@jankapunkt
Copy link
Member

Seems to be introduced in #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
Copy link
Collaborator Author

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
Copy link
Member

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

@bratelefant
Copy link
Collaborator Author

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 jankapunkt linked a pull request Jul 12, 2023 that will close this issue
@jankapunkt
Copy link
Member

@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
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Contributor

ToyboxZach commented Jul 14, 2023

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
Copy link
Collaborator Author

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
Copy link
Contributor

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Member

jankapunkt commented Aug 1, 2023

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

@jankapunkt
Copy link
Member

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

  • making this library offline-first would be a big and IMO positive improve
  • we have the following use-cases
    • app goes to background
    • app comes from background
    • app is closed / exited
    • app is started
  • 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?

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

@bratelefant
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Collaborator Author

bratelefant commented Aug 8, 2023

  • 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
Copy link
Collaborator Author

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working no-issue-activity
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants