[USERS SUB-PR] Changes profile loading logic

[mike-lvov]

#1047 working on this PR, I have discovered that when there was even a lightest delay for users fetching, it produced "one-by-one addition to the store" bug.

While I was trying to figure out why that was happening (I thought I solved that by having one query of users), I spent a few hours debugging that. My finding was that the profile store was actually adding the first item to the users store. It means that if we load the profile first, it will update the users store to this worldUsers: [{.MY_PROFILE_OBJECT }]. Because of this lib's optimization, I had to find a workaround for this.

This PR is a draft proposal. I don't see any other way to handle this so far, I'm opened to any suggestions since I spent the whole day trying to figure out the way to handle this.

[mike-lvov]

Just got an enlightening that it won't work for new users since they have a different flow(where the profile is probably getting fetched before the users)

[0xdevalias]

This relates to the bug in prescottprue/react-redux-firebase#914

In prescottprue/react-redux-firebase#914 (comment)

As seen here docChanges is being used to dispatch changes to documents if they exist and the updates size is less than the full query size. This was originally to handle updates which are triggered on only a single document or sub-group of documents instead of the whole list of docs.

There is a TODO there to make this optional, but wondering if that would cause any issues when only a single doc is changed.

The reason we get the 1 by 1 updates is the forEach loop here, which emits docChangeEvent events (which uses the DOCUMENT_MODIFIED action type constant), though the 'cause' of that may be somewhere else in the library:

• dispatchListenerResponse: https://github.com/prescottprue/redux-firestore/blob/d7a561361fecff128b35cf37712f852637a66f41/src/utils/query.js#L762-L768
• docChangeEvent: https://github.com/prescottprue/redux-firestore/blob/2a2611100ec603d8f658e55431af77bea3b840f5/src/utils/query.js#L714-L736

dispatchListenerResponse is called from the following location, and docData is the param we care about, which comes from the firestoreRef(firebase, meta).onSnapshot( handler:

• https://github.com/prescottprue/redux-firestore/blob/2a2611100ec603d8f658e55431af77bea3b840f5/src/actions/firestore.js#L172-L213

firestoreRef is a wrapper/helper around firebase.firestore that doesn't really seem to do much, so can likely be ignored:

• https://github.com/prescottprue/redux-firestore/blob/2a2611100ec603d8f658e55431af77bea3b840f5/src/utils/query.js#L144

Which means we are mostly just dealing with the firebase.firestore.onSnapshot() method from the Firebase SDK:

• https://firebase.google.com/docs/firestore/query-data/listen

  • You can listen to a document with the onSnapshot() method. An initial call using the callback you provide creates a document snapshot immediately with the current contents of the single document. Then, each time the contents change, another call updates the document snapshot.

    • Since the initial call creates a snapshot immediately with the current contents of the document, I would be looking at what data exists within that that we could use to 'identify' this initial snapshot (I expect that is the intent of the if (docChanges && docChanges.length < docData.size) { line in the lib)

---

In prescottprue/react-redux-firebase#914 (comment):

It seems to be because of how Firebase's SDK is returning that data if it already has been loaded by the SDK in another listener.

If that comment is true, then it sounds like it would be possible to make a minimal test case to 'reproduce' this bug, where you have 2 firebase.firestore.onSnapshot() listeners that connect to the same thing, and then from his comment there, I would expect that the initial event on the 2nd listener wouldn't contain the full 'initial snapshot' as the docs describe.

@mike-lvov This would be the area I would probably focus on looking at at first, since that would probably prove/disprove that that would be the issue

https://firebase.google.com/docs/firestore/query-data/listen#view_changes_between_snapshots

• It is often useful to see the actual changes to query results between query snapshots, instead of simply using the entire query snapshot.

• Important: The first query snapshot contains added events for all existing documents that match the query. This is because you're getting a set of changes that bring your query snapshot current with the initial state of the query.

• The initial state can come from the server directly, or from a local cache. If there is state available in a local cache, the query snapshot will be initially populated with the cached data, then updated with the server's data when the client has caught up with the server's state.

---

If we can't figure out how to fix it in the library, we could quite probably just set up our own onSnapshot listener, that emits the same sorts of data update events/similar. This is actually very close to the 'workaround solution' I proposed the other day, where we could set up our own listener in a 'singleton context' and/or hook (though if we can fix the library in a reasonable amount of time, that is my preference)

---

By looking at where the DOCUMENT_MODIFIED event is handled, we can check what will likely be impacted by making any changes here:

• https://github.com/prescottprue/redux-firestore/search?q=DOCUMENT_MODIFIED

These are the places I would start looking if I were trying to solve this bug in the library itself.

[0xdevalias]

@mike-lvov can you please add a comment explaining why you're closing PR's when you do, so that we have some context captured.

[mike-lvov]

@0xdevalias This PR became unrelated since in this PR I try to find a workaround for
prescottprue/react-redux-firebase#914, but this PR
prescottprue/redux-firestore#319 solves the issue. So, no need for workarounds

[0xdevalias]

@0xdevalias This PR became unrelated since in this PR I try to find a workaround for

prescottprue/react-redux-firebase#914, but this PR

prescottprue/redux-firestore#319 solves the issue. So, no need for workarounds

Thanks for that :) The context just helps if we come back to it in future and wonder why it was closed. This way we have a simple summary to sum it all up and don't need to look deeper.