Proposal (Firestore): better default behaviour for the `collectionData` and `docData` functions (for the underlying SDK's handling of optimistic updates)

[jits]

tl;dr

Make a technically breaking change to the collectionData and docData functions to always have includeMetadataChanges set to false when listening for snapshots.

Background

The underlying Firebase SDK for Firestore performs optimistic updates on the client, with a hasPendingWrites metadata flag available on snapshots to tell you when a server-side update has been committed (only if you've called the underlying onSnapshot with includeMetadataChanges: true, which is what rxfire currently does). From the docs:

Local writes in your app will invoke snapshot listeners immediately. This is because of an important feature called "latency compensation." When you perform a write, your listeners will be notified with the new data before the data is sent to the backend.

Retrieved documents have a metadata.hasPendingWrites property that indicates whether the document has local changes that haven't been written to the backend yet.

Source: https://firebase.google.com/docs/firestore/query-data/listen#events-local-changes

This behaviour results in a very common "why am I seeing two updates trigger for my snapshot?" (though only when includeMetadataChanges is set to true). The idea is that you could filter out and choose which of the updates to respond to in your implementation (or something more elaborate, e.g. to handle offline scenarios).

Note: it's not clear (at least, to me) what happens if a server-side error occurs (e.g. security rules fail) but I'll assume for now that a failure always results in a new snapshot being emitted.

Problem: double updates emitted when using the rxfire collectionData and docData functions with no way to filter out

In the current rxfire implementation, the collectionData and docData functions end up setting includeMetadataChanges: true when setting up the snapshot listener. So if an update is made to any document in the collection (or to the single document) being observed then two whole emissions of the collection (or document) are made in the observable – one for the client-side optimistic update and another once the server-side update has committed. This has been reported previously in: #50.

This would normally be okay except the emitted value of both functions do not have the hasPendingWrites metadata flag, so it's impossible to choose when to filter out one of the emitted results. And since there's no way to explicitly set includeMetadataChanges to false when listening to the snapshot we have to live with this behaviour (with some folks resorting to workarounds).

Note: the same behviour is true of the collection and data functions, but in those cases we do have access to the hasPendingWrites flag so we have control over which we filter out.

Proposed solution

An attempt was made to allow configuring the includeMetadataChanges option in #12.

Instead of the approach taken there, this proposal suggests:

• Changing the default behaviour of the collectionData and docData rxfire functions to always have includeMetadataChanges set to false when setting up the snapshot listener.
  • This is technically a breaking change but may not have a big impact as the net effect is that the same results will be emitted once, rather than twice. So it's only a "breaking" change if folks were relying on this double update behaviour (as opposed to treating it like a bug).
  • Rationale: these methods are essentially convenience methods on top of the underlying SDK, which simplify usage (e.g. setting the ID field). So it could make sense to also abstract away the metadata change and use the default behaviour of the underlying SDK.
• And as a result of this implementation change (see below), allowing the collection and doc rxfire functions to be passed in a value for includeMetadataChanges, defaulting to true (as it is now).

Implementation details

The current implementation chain goes like (shown for collection, but the same for doc):

sequenceDiagram
    collectionData (rxfire)-->>collection (rxfire): ...
    collection (rxfire)-->>fromRef (rxfire): includeMetadataChanges: true
    fromRef (rxfire)-->>onSnapshot (SDK): ...

Loading

Proposed solution:

sequenceDiagram
    collectionData (rxfire)-->>collection (rxfire): includeMetadataChanges: false
    collection (rxfire)-->>fromRef (rxfire): includeMetadataChanges: false<br/>but could be true if collection called directly
    fromRef (rxfire)-->>onSnapshot (SDK): ...

Loading

/cc @davideast

[jits]

Hi @davideast 👋🏽 I expect you're busy with a million things right now — do you think there's a possibility we could pick up this change and works towards a new (breaking change) version of rxfire with improvements like this? Thanks!