sync problem with a collection and document of overlapping paths

[mesqueeb]

there seem to be some sync issues when there's a collection called users and a doc called users/{userId} both loaded at the same time.

problem 1. not all users get loaded

problem 2. user doc is not updated when that doc is updated through the users module and visa versa

related: #316

[mesqueeb]

From @dsl101

I think this may have had some unintended consequences... I've got a collection with over 100 user documents in it, and 2 modules using v-e-f.

The first module (fsUser) binds the currently logged-in user to the document at /users/{userId}. It can then test if the current user is an admin allowed to edit other user accounts. If so, it binds a second module (fsAllUsers) to the collection at /users.

But, what I'm seeing is that this second module only sees the current user document, not all 100+ users. And the reason is this change I think.

I added some logging to my local copy of the v-e-f package like this:

else if (gotFirstLocalResponse) {
    // it's not the first call and it's a change from cache
    // normally we only need to listen to the server changes, but there's an edge case here:
    // case: "a permission is removed server side, and instead of this being notified
    // by firestore from the _server side_, it only notifies this from the cache...
    if (getters.collectionMode) {
        docChanges = querySnapshot.docChanges();

console.log('docChanges:', docChanges)

        docChanges.forEach(function (change) {
            // only do stuff on "removed" !!
            if (change.type === 'removed') {
                var doc = getters.cleanUpRetrievedDoc(change.doc.data(), change.doc.id);
                dispatch('applyHooksAndUpdateState', {
                    change: change.type,
                    id: change.doc.id,
                    doc: doc,
                });
            }
        });
    }
}

and I can see in that console.log() line that there are 110 entries in docChanges. But then they are ignored, as they're all of type added...

I suspect this is because I already have the /users/{userId} document bound, and perhaps that is changing the behaviour of the firebase library? If I target a completely unrelated collection, I get all the docs as expected. But in this case, I do need to bind it twice in different modules (one as a doc, one as a collection).

If I comment out the line if (change.type === 'removed') { and its closing brace, I get all the docs in the collection.

However, I'm also missing updates on the collection module (e.g. changes in the bound document are synced with firestore, but then not replicated back to the bound collection), so perhaps this isn't the whole story. Should it be possible to have these 2 modules running happily side-by-side? If so, I'll try to make a minimal repro.

[mesqueeb]

I'm having difficulties trying to reliably reproduce this.

The times there's no problem for me:

The times there's a problem:

I have no idea of the cause yet.

[mesqueeb]

added more info

working case:

failing case:

[dsl101]

If it's any consolation, I also see the weirdness when sometimes it works fine and sometimes it doesn't. There seem to be different mechanisms the firebase SDK uses in different scenarios—for example, I'm developing a Vue/Quasar app, and hot-reloaded changes see different results to refreshing the browser page, to refreshing with clear cache, to logging out and in again (firebase authentication).

In my original post on the other thread, the console.log() shows 110 changes of type 'added', and so bypassing the if (change.type === 'removed') test and calling dispatch('applyHooksAndUpdateState', ... on all of them did appear to fix things though. Is it possible that code path should be used for more change types?

[mesqueeb]

on a local build, I comment out the entire part at:

else if (gotFirstLocalResponse) {
// it's not the first call and it's a change from cache

I'm now 100% sure it's not related...

If I, after auth listener triggers I execute:

dispatch('company/openDBChannel', { companyId }, { root: true }).then(() => {
  dispatch('companies/openDBChannel', null, { root: true })
})

then 50~80% of the time the second call will fail. that "companies" collection will execute with fromCache set to true twice...

If I instead do:

dispatch('company/openDBChannel', { companyId }, { root: true })
dispatch('companies/openDBChannel', null, { root: true })

just in parallel, then it works 100% of the time........

[mesqueeb]

@dsl101
Ok, I found out it will fail no matter how long you wait, as long as you do something like this:

dispatch('company/openDBChannel', { companyId }, { root: true })
setTimeout(() => {
  console.log(`waited 5 sec!`)
  dispatch('companies/openDBChannel', null, { root: true })
}, 5000)

now it's reproducible and someone can perhaps report to firebase js sdk. :D maybe I will if I have some time later!

[mesqueeb]

tracked down the bug to the firebase sdk level. I hope to find a workaround soon
firebase/firebase-js-sdk#3053 (comment)

[dsl101]

I've also been able to reproduce it in the browser—but apart from that fromCache flag, everything else in the event seems correct (e.g. docChanges() contains 1 element on the first call, and then n-1 elements on the second call, and they have the correct data in them).

I notice someone just picked up your issue though, so that's promising :)

[mesqueeb]

Making some progress in the firebase thread. Hope that they can look into it.

Based on their solution/answer I will either way have to review how my library handles what happens in the onSnapshot listener.

[louisameline]

One thing is odd in the bug report. You say a prerequisite is to have persistence disabled (no cache). Then how could the first trigger batch of data come from cache? Did I miss something?

[louisameline]

Apart from this oddness, I'm starting to wonder if there actually is an issue. Yes, it's weird that sometimes it starts by a fromCache=true response if there is no cache, but when it does, there is a follow-up fromCache=false response which basically tells you "I have now checked with the server and there are no doc changes since the last times I gave you the docs from cache".

[louisameline]

Basically, the issue is more about having two realtime listeners with a same doc in common, and only one of them triggering an update, right? Maybe because the second one comes "from cache" from Firestore's point of view, and that we haven't thought of that case. Or maybe Firestore doesn't trigger anything at all on the second listener because somehow it considers we've been informed already. Can you please check with metadata changes enabled?

As a side note, in the next version of vef, that won't be an issue anyway, as docs of a collection will be modules on their own, so there wouldn't be two modules storing the same data (which is a bad design choice to begin with imho, even if you felt it was simpler to handle at the time).

[louisameline]

This being said about problem 2: I'm more surprised by problem 1. When you say

problem 1. not all users get loaded

What is it about exactly? One missing (which one, the one streamed in the other module)? Or several?

[mesqueeb]

@dsl101
I agree with @louisameline that:

problem 2. user doc is not updated when that doc is updated through the users module and visa versa

this is a problem with how vuex-easy-firestore 1.0 is built and is difficult to fix with the current architecture. We'll mark this as won't-fix and make sure this doesn't happen in v2.0, which is coming along slowly but surely.

@louisameline

You say a prerequisite is to have persistence disabled (no cache). Then how could the first trigger batch of data come from cache? Did I miss something?

Yeah, I have no idea. I guess that's just how firebase works. It sometimes gives the server results marked fromCache: true while they are actually coming from the server. And enabling includeMetadataChanges will not prevent this, you'll 100% get another fromCache: false trigger in this case, but without sometimes without any documents inside.

You can replicate this sad truth about the weirdness of firebase, unrelated to v-e-f by following my reproduction steps here:
firebase/firebase-js-sdk#3053 (comment)

Now, about problem 1...

@louisameline
I think we have a really clear answer now in the last post of wu-hui:
firebase/firebase-js-sdk#3053 (comment)

I reviewed the code we have in place and I think, if we move the check for pending writes up and do nothing in that case:

if (hasPendingWrites === true) {
  // do nothing
} else if (fromCache === true) {
  // always update docs in vuex state
} else {
  // always update docs in vuex state
}

Then we don't need to listen for the gotFirstLocalResponse anymore and can delete this variable.

An abstraction looks something like this:

However, the gotFirstLocalResponse is something you have added 5 months ago:
https://github.com/mesqueeb/vuex-easy-firestore/blame/dev/src/module/actions.ts/#L630

We need to keep in mind:

1. Make sure we don't break those issues you beautifully mention in the commit (so handy!) 😄

2. Also, I think perhaps we don't need any difference between fromCache === true and false at all, as long as we filter out hasPendingWrites calls.
   Especially because, as we've seen, just opening a sub-document of a collection separately can make it so the collection has 2 subsequent fromCache === true triggers.
   (the first one from the sub-document that was cached, the second from the rest of the docs that apparently are still returned with fromCache === true)

3. Another place I found we need to review is where this comment is written:

// the promise should still be pending at this point only if there is no persistence,
// as only then the first call to our listener will have fromCache === false

Because this is simply not true as we've found out.

I'll hit you up on discord, let's discuss!

[dsl101]

I built a small test-case using native firebase SDK rather than vef, and, ignoring all the extra bells and whistles that vef provides (I'm only really interested in online, so no persistence, no metadata changes, etc.), I could bind a document and a collection separately, change either (or change it from the firebase console), and see all the changes I needed to see by simply responding to every onSnapshot(). If vef worked this way, and with the recent update that ensured only changed Vuex state that had actually changed, avoiding needless Vue reactivity updates, the worst case scenario for me is a little wasted time comparing the latest docChanges to what we already have in Vuex. That seems like a small price to pay for simplicity.

To be honest, I'd even go for simply updating firebase (rather than patching Vuex locally first, and therefore saving the complication of rolling those local changes back on error). So it might be the case that I really don't need vef, but I'm always keen to use packages rather than re-invent the wheel; it may just be more than I need.

I'm curious about the point of making collections bind each individual doc as a module—I'm dealing with collections of many hundreds of docs, and thinking the overhead of a module per doc could be high? But, if not, then perhaps that's an option in the medium term when V2 is available.

[louisameline]

the worst case scenario for me is a little wasted time comparing the latest docChanges to what we already have in Vuex

We'll adapt yes and there already are cases where we know there will be unnecessary hook triggers indeed.

To be honest, I'd even go for simply updating firebase rather than patching Vuex locally first

Absolutely, we are aware of that. Optimistic UI should be reserved to things that are easy to rollback. V2 will allow for the remote update before triggering the local update. And transactions. And many, many other cool things.

I'm curious about the point of making collections bind each individual doc as a module

Currently, when you have a collection, you trigger actions on the collection to interact with one of its documents. The collection's document config, so to speak, is defined in the collection module config. This alone is already awkward if you think about it, and inconsistent with document modules.
Besides, what if a collection document has a subcollection? Will we have the subcollection config in the collection module config too?
The solution is ideally to follow a simple principle: 1 module for 1 resource.
There were other reasons to go down that path, maybe even better ones, but these come from the top of my head.

the overhead of a module per doc could be high

Yes, we are aware of this, when you have hundreds of documents. I'm not sure what we'll do for that yet. Maybe keep the current behavior possible via an option, but not advertised as the "normal" way to handle things. The data would be kinda "read-only" though, and we'd instantiate a document module on-demand when you actually want to interact with a specific document. To be continued :)

[dsl101]

Just a question—how much of this complexity is due to managing the 'optimistic' update of the local state before sending data to firestore? I ask only because I've coded up a trivial implementation of vef to test against (all it does is bind a doc or collection, process snapshots, and allow updates via the firebase library calls). And I've noticed that the default behaviour seems to be to receive an onSnapshot() event with hasPendingWrites: true immediately upon calling doc(id).set() etc. So the UI is 'optimistic' anyway, in that the state is updated practically instantly (certainly not enough delay to be noticeable).

Perhaps there's duplicated effort here in this case—my trivial library almost entirely suits my needs so I may just stick with it. Certainly it's not as comprehensive as vef, but I'm not using any hooks, persistence, or other 'advanced' features of firestore that vef abstracts.

[louisameline]

I don't understand what you are asking for?
Firestore itself promotes optimistic behavior as the default way of handling things indeed.

Anyway, you can always reinvent the wheel of course. It will be limited, buggy, you'll have to maintain it alone and you'll miss so many things you don't know you need yet, but it's possible.

[mesqueeb]

@dsl101 it's always healthy to weight the pros and cons of introducing a library into your codebase. To see if it does what you need it to and if it doesn't bloat your code too much. In your case perhaps writing your own library specific for your use case, is definitely a good option. However, as Louis mentioned, it might lead to you having to spend more time, doing something we already did.

When it comes to this bug, today I implemented the onSnapshot listener for the next version of the library. Now I'm in a good spot to tweak the listener for version 1.0 so that this bug is fixed once and for all! I'll probably push the update tomorrow or the day after! 😉

[dsl101]

You seem a little offended by my comment—certainly that wasn't my intention. As for reinventing the wheel, why not use vuexfire, which has been around since 2016 and is now part of the vuejs repo, rather than write vef? And I guess we wouldn't be here if vef was bug free...

The point is to use exactly the right tool for the job. I'm not reinventing the wheel, just putting the correct tyres on. If my app grows in complexity to the point where I need lots of additional capability offered by vef or any other library, I will certainly assess that. In the meantime, I'm still keep to improve vef, understand what's going wrong here, etc.

I think my question was a misunderstanding of this issue where @mesqueeb said "vuex easy firestore already has optimistic UI built in". I'd taken that to mean the vef was patching the vuex state in advance of getting the onSnapshot() from firestore. Having looked at the vef code, I'm not entirely sure if it is, but in any case there's a fair amount of complexity there which led me to think that building from first principles would allow me to understand and compare. And when I did, I saw that firestore appears to have that optimisation itself (perhaps that's what Luca was referring to, perhaps not).

In any case, it turned out that my simple wrapper around the firestore SDK served my app's purpose, so why use something more complex than needed?

[mesqueeb]

Hey @dsl101 a quick note on the current status of the optimistic UI:

• write actions:
  They update the local data in vuex first, then they start a queue to be synced to firestore. If there are no further api calls within 1000ms, the data is synced to firestore, if there are, it will reset the queue each time back to 1000ms and gather all updates into a single batch.

When it comes to the actual firebase SDK with which we make the batch call, that has its own cached data (used for it's native persistence etc.). The firebase SDK will in turn as well, update it's own local cache first, then sync to the server.

Basically, when working with v-e-f, you end up with local data twice: once in vuex and once in the Firebase SDK. the difference is, the firebase SDK charges you each time you try to read that data, because it can only be read through get() and onSnapshot() calls, and will each time give you back its cache and then also make the server call.

Which leads me to my next point:

• read actions: openDBChannel
  An onSnapshot listener just waits for data coming from the server, then updates vuex based on that. the onSnapshot listener gives us its cached data first, which we then take and put into vuex, and then immediately after, firestore gives us an empty array with confirmation that the data is up to date with the server, or those docs that came from the server and had differences with its own local cache.
  V-e-f will basically keep updating vuex with whatever comes in, but ignore if there are changes gives to us by the onSnapshot listener that were made in the client itself. Because this would cause duplicate updating in vuex and causes all sorts of issues.

I learned now that I made a small mistake in the past with how I handle the onSnapshot listener which manifested in this bug. But now I know how to fix it, so I'll do it very soon, hold on just 1-2 more days!

• read actions: fetchAndAdd
  fetchAndAdd is the same as firestore's get() and then add that data to Vuex. It's not optimistic, since it's just the single api call to get data from the server, then add it to Vuex.

I hope this helps! 😉

PS: the reason I didn't use Vuex Fire and started building Vuex Easy Firestore, is because Vuex Fire had very bad documentation, and I wanted a different approach than what Vuex Fire was already doing. Also at that time I needed to have the flexibility to make sure it fits all my own needs, so I decided to just write something that fits all my needs. 😄 Then after a while I realised, I can open source my work so far, so others can enjoy it, and so vuex-easy-firestore was born.

[louisameline]

@dsl101 I'm not offended at all, no worries. I was just surprised that the advantages of v-e-f compared to Vuexfire were still unclear to you at this point. We'll definitely need to update the doc to make the advantages clear to everyone.

From the top of my head, I'd say the key points are:

• no Firestore SDK calls all over your code, no real need to learn how they work. You just manipulate your store as you are used to, with just the extra fetchAndAdd actions, that's all. Someday, the lib might be connected to other providers (MongoDB, etc.), and you might not have to change your own code at all, just setup a different connection. All you really should care about is your local store.
• use your app with replication turned off. Imagine your user stops paying his subscription, and that you want to let him use a limited version of your app locally, with no remote backup. If you have to bypass every SDK call in your code, you can imagine the mess. This feature is currently not included in v-e-f but I have personnaly tweaked it to work that way, and it should come in future versions.
• ease of use. There are many built-in things in vef that save you trouble, and upcoming optimizations that will even save you money, by making as few calls as possible.
• It's been a while since I used Vuexfire, but I think it couldn't be used offline. I briefly gave a look at the doc just now and it seems they say nothing explicit about how it works, but it might be opening DBChannels for every document you bind onto. And if you don't wait for promises to resolve, it's just like us: it works on Firestore's cache and their sentence "Your store will always be in sync with your remote database" no longer applies, and you have to deal with failures and conflicts as well. There is no silver bullet with Firestore: if you want to play smart, you need a lib that gives you all the options.

And there is other stuff in the works. Vef is clearly young and perfectible, but at some point there will be no possible doubt that it plays at another level.

firestore appears to have that optimisation itself

When you enable Firestore persistence, and do things synchronously after a call to a Firestore SDK method (as usually indicated in their doc), you get "optimistic UI" yes. V-e-f mostly relies on that pattern for now, that's why v-e-f is also optimistic. Or you can wait for the promise returned by the method to resolve, for confirmation of the remote update.

[dsl101]

Tx for the replies—I've used vuexfire for that last few years with firebase, and decided to give vef a try when porting the app to firestore. But there are a number of things quite specific to my use case that do probably mean vef is more than I need right now (and probably the same is true of vuexfire since I didn't really know firebase / firestore very well at all when I started this). I don't need anything offline, and I'm only using onSnapshot() listeners (no single fetch, etc.). Everything is driven by individual user interactions that need replicating to other users asap at this point, so no throttling benefit, etc. No Electron / Cordova requirement, and so on. I can see vef takes all of these things out of the way, and certainly in another use case I can see how useful that would be.

Having said all that, I'm still keen to pin down the optimistic bit. I'm not using persistence, just a simple call to onSnapshot(), and when I then send a .doc().update() type call to the firestore SDK, I see the onSnapshot() handler triggered with {hasPendingWrites: true} far faster than I would expect a network round trip. To me, this indicated that firestore was triggering my local UI code to update before actually writing that new data to the server. And if that succeeds, that's the last I hear from the firebase SDK for that update. So my UI is up-to-date fast, and only a single write to firestore. Is that how you understand it works?

[louisameline]

Yes, that's how it works. Even if you're offline, onSnapshot() handler is triggered immediately with {hasPendingWrites: true}

that's the last I hear from the firebase SDK for that update

Yes, unless you hold on to the promise that resolves after the actual update, and/or ask for metadataChanges on the snapshot, in which case your handler will also be notified that the update has been carried on.

With mesqueeb, there are a couple edge-case scenarios with weird behavior that we still need to understand, but that's roughly it. Check their doc, they give some info about that.

[mesqueeb]

@dsl101 problem 1 is fixed in latest version. problem 2 won't be fixed for v1.0 because it requires a complete rewrite of about half of the library, but it's already implemented for v2.0.

[louisameline]

I'm not sure about the new way openDBChannel works now. Specifically, this line:

if (docSnapshot.metadata.hasPendingWrites) return

Imagine that you request a write to Firestore, and that it takes 5 seconds to perform.
If a modification is made during these 5 seconds, I suspect you'll get a snapshot with the changes as expected, but I guess it will have the hasPendingWrites flag, so you'll ignore it.

In the issue you raised to the Firebase team, I believe there was nothing wrong in your screenshot when includeMetadataChanges was true. At the end when Wu-hui says "Yes, that should work", I think he only means "yes, you'll know at that time that your changes have been carried out", but he might have overlooked the side effect I mentioned.

I don't really understand what your problem was to begin with, I'd need a reproduction snippet, but I think it might only happen when includeMetadataChanges is false. Does it occur when you set it to true?

It's obvious to me why it wouldn't work when it's false, so I feel stupid for letting this happen in the first place, but my pending PR solved this since later as includeMetadataChanges is always true there.
Apart from this includeMetadataChanges === false issue, I can't see what would be wrong in my latest code. To be honest, I haven't learned anything from the discussion with Wu-hui, so it doesn't give me a clue about the need to change anything? Or I might just need more info on your issue.

[mesqueeb]

Yeah, I'll force includeMetadataChanges: true and re-deploy, that seems OK.

The cases I imagine you are thinking of is:

• have a working refreshed promise - this has a chance to not trigger with the current implementation
• the dev uses serverTimestamp() and the local state needs to be updated with the timestamp after the server write has finished. includeMetadataChanges will make sure it triggers when the timestamp is filled in.

For me the main take-away from that thread was:

• we were listening to fromCache to determine if something comes from the server or not, while we should listen to hasPendingWrites to determine if something comes from the server or not. (it seems all the documentation now also only mention hasPendingWrites, and no-where besides the "reference" I can find info on fromCache, not sure if this was different two years ago)

[mesqueeb]

@louisameline I have reviewed your PR as well, but since the code of the onSnapshot listener is now very different it's difficult to merge. However, I have enabled includeMetadataChanges: true by default now in the latest version.

The part where you are comparing snapshots I believe is already addressed by my changes here a while ago:
128c639#diff-a54597677f9b53b437662eb618319e04R31
The change I made there is that the Vuex store's data is only updated where and when the data or nested data is different from what is already there.

[louisameline]

The way I understood it, hasPendingWrites doesn't mean that the data comes from local, it means that there are changes you have locally that haven't yet been saved remotely. Hence the likely problem I said previously. But we should ask to be sure.

I remember we weren't using fromCache before I refactored the function, we were using hasPendingWrites and it didn't work well, but I guess it wasn't the sole issue.

For the merge, if I'm right about hasPendingChanges, we can revert your changes and merge mine. I think there was another previous commit you made that diverged, I'll check what it was and incorporate. I'll be working on that this morning

[louisameline]

the code of the onSnapshot listener is now very different it's difficult to merge

I checked and I think my openDBChannel function can just be pasted on top of the existing code, with these adjustments to be made again:

• some indents and spaces you changed
• add the 4 TS types you created
• split the snapshot handler in 2 again (doc/collection), if really necessary (not sure it is)

My PR cleaned a few things on its own, it would be nice to have them merged.