FirebaseExtended / FirebaseExtended/rxfire

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

Open
#84 1 comment 2 reactions 0 assignees View on GitHub
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.