FirebaseExtended / FirebaseExtended/rxfire
Proposal (Firestore): better default behaviour for the `collectionData` and `docData` functions (for the underlying SDK's handling of optimistic updates)
- Dominant language
- TypeScript
- Stars
- 149
- Forks
- 50
- PR merge metrics
- No merged PRs in 30d
Description
## 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:
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: .
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](https://github.com/FirebaseExtended/rxfire/issues/50#issuecomment-1146832270)).
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 .
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):
```mermaid
sequenceDiagram
collectionData (rxfire)-->>collection (rxfire): ...
collection (rxfire)-->>fromRef (rxfire): includeMetadataChanges: true
fromRef (rxfire)-->>onSnapshot (SDK): ...
```
Proposed solution:
```mermaid
sequenceDiagram
collectionData (rxfire)-->>collection (rxfire): includeMetadataChanges: false
collection (rxfire)-->>fromRef (rxfire): includeMetadataChanges: false
but could be true if collection called directly
fromRef (rxfire)-->>onSnapshot (SDK): ...
```
/cc @davideast
Contributor guide
Assessment
This issue has not been assessed yet.