Fix disposable of optimistic updates

Summary:
The basic flow of the bug (as perceived by the user) is:
1. Apply an optimistic update, updating some subscriber S (e.g. a useFragment caller) w new data.
2. Commit a final update (causing the optimistic update to get rebased)
3. Dispose the optimistic update
4. Observe that subscriber S is not correctly notified of the updated state

## The Bug

* In step 1 we create a backup value for S that reflects its state pre-optimistic update
* In step 2 we:
  * Restore this backup
  * Commit the new changes (notably, w/o recalculating each subscription's results)
  * Snapshot the store prior to rebasing the optimistic update.

The last snapshot step is the problematic part: the snapshot used for the backup is the *current value of the subscription*, but this value hasn't been updated to reflect the current state of the store. Note that in practice most final operations will affect the same data that was affected by the optimistic updater, ensuring that the subscription would be recalculated correctly. The bug occurs only when the "final" update is empty (or more specifically, it occurs for all subscriptions affected by the optimistic update but that aren't affected by the final update).

## The Fix

The fix is to ensure during the store snapshot that we re-read the backup for stale subscriptions, ensuring that we have a correct backup to return to later. Non-stale subscriptions didn't diverge between their last backup/current value, so we can use their current value as the backup.

## Performance Implications

This adds a re-read of a subset of subscriptions during the store snapshot operation - which only occurs when rebasing optimistic updates. This should be a relatively small overhead, and its required for correctness so in the short-term I'd argue we should pay the cost and evaluate.

Reviewed By: tyao1

Differential Revision: D19539900

fbshipit-source-id: f684d2b0c7d46e7015d766a2ee819a5be8906bd7

packages/relay-runtime/store/RelayModernStore.js

@@ -456,7 +456,27 @@ class RelayModernStore implements Store {
         'snapshot exists.',
     );
     this._subscriptions.forEach(subscription => {
-      subscription.backup = subscription.snapshot;
+      // Backup occurs after writing a new "final" payload(s) and before (re)applying
+      // optimistic changes. Each subscription's `snapshot` represents what was *last
+      // published to the subscriber*, which notably may include previous optimistic
+      // updates. Therefore a subscription can be in any of the following states:
+      // - stale=true: This subscription was restored to a different value than
+      //   `snapshot`. That means this subscription has changes relative to its base,
+      //   but its base has changed (we just applied a final payload): recompute
+      //   a backup so that we can later restore to the state the subscription
+      //   should be in.
+      // - stale=false: This subscription was restored to the same value than
+      //   `snapshot`. That means this subscription does *not* have changes relative
+      //   to its base, so the current `snapshot` is valid to use as a backup.
+      if (!subscription.stale) {
+        subscription.backup = subscription.snapshot;
+        return;
+      }
+      const snapshot = subscription.snapshot;
+      const backup = RelayReader.read(this.getSource(), snapshot.selector);
+      const nextData = recycleNodesInto(snapshot.data, backup.data);
+      (backup: $FlowFixMe).data = nextData; // backup owns the snapshot and can safely mutate
+      subscription.backup = backup;
     });
     this._optimisticSource = RelayOptimisticRecordSource.create(
       this.getSource(),

packages/relay-runtime/store/__tests__/RelayModernEnvironment-ApplyUpdate-test.js

@@ -140,4 +140,72 @@ describe('applyUpdate()', () => {
       name: 'zuck',
     });
   });
+
+  it('notifies the subscription when an optimisitc update is reverted after commiting a server response for the same operation and also does not update the data subscribed', () => {
+    const selector = createReaderSelector(
+      UserFragment,
+      '4',
+      {},
+      operation.request,
+    );
+    const callback = jest.fn();
+    const snapshot = environment.lookup(selector);
+    expect(snapshot.data).toEqual(undefined);
+    environment.subscribe(snapshot, callback);
+
+    const disposable = environment.applyUpdate({
+      storeUpdater: proxyStore => {
+        const zuck = proxyStore.create('4', 'User');
+        zuck.setValue('4', 'id');
+        zuck.setValue('zuck', 'name');
+      },
+    });
+    expect(callback.mock.calls.length).toBe(1);
+    expect(callback.mock.calls[0][0].data).toEqual({
+      id: '4',
+      name: 'zuck',
+    });
+
+    callback.mockClear();
+    environment.commitPayload(operation, {
+      me: null,
+    });
+    expect(callback.mock.calls.length).toBe(0);
+
+    disposable.dispose();
+    expect(callback.mock.calls.length).toBe(1);
+    expect(callback.mock.calls[0][0].data).toEqual(undefined);
+
+    callback.mockClear();
+    const disposable2 = environment.applyUpdate({
+      storeUpdater: proxyStore => {
+        const zuck = proxyStore.get('4') ?? proxyStore.create('4', 'User');
+        zuck.setValue('4', 'id');
+        zuck.setValue('Mark', 'name');
+      },
+    });
+    expect(callback.mock.calls.length).toBe(1);
+    expect(callback.mock.calls[0][0].data).toEqual({
+      id: '4',
+      name: 'Mark',
+    });
+
+    callback.mockClear();
+    environment.commitPayload(operation, {
+      me: {
+        id: '4',
+        name: 'Zuck',
+      },
+    });
+    // no updates, overridden by still-applied optimistic update
+    expect(callback.mock.calls.length).toBe(0);
+
+    callback.mockClear();
+    disposable2.dispose();
+    expect(callback.mock.calls.length).toBe(1);
+    expect(callback.mock.calls[0][0].data).toEqual({
+      id: '4',
+      name: 'Zuck', // reverts to latest final value
+    });
+  });
 });