Don't rebroadcast for failures

zino-hofmann · Aug 3, 2024 · 20f6247 · 20f6247
1 parent 81c4f65
commit 20f6247
Show file tree

Hide file tree

Showing 3 changed files with 93 additions and 3 deletions.
diff --git a/packages/graphql/lib/src/cache/cache.dart b/packages/graphql/lib/src/cache/cache.dart
@@ -204,9 +204,23 @@ class GraphQLCache extends NormalizingDataProxy {
   ///
   /// This allows for hierarchical optimism that is automatically cleaned up
   /// without having to tightly couple optimistic changes
+  ///
+  /// This is called on every network result as cleanup
   void removeOptimisticPatch(String removeId) {
+    final patchesToRemove = optimisticPatches
+        .where(
+          (patch) =>
+              patch.id == removeId || _parentPatchId(patch.id) == removeId,
+        )
+        .toList();
+
+    if (patchesToRemove.isEmpty) {
+      return;
+    }
+    // Only remove + mark broadcast requested if something was actually removed.
+    // This is to prevent unnecessary rebroadcasts
     optimisticPatches.removeWhere(
-      (patch) => patch.id == removeId || _parentPatchId(patch.id) == removeId,
+      (patch) => patchesToRemove.contains(patch),
     );
     broadcastRequested = true;
   }

diff --git a/packages/graphql/lib/src/core/observable_query.dart b/packages/graphql/lib/src/core/observable_query.dart
@@ -95,8 +95,19 @@ class ObservableQuery<TParsed> {
   /// call [queryManager.maybeRebroadcastQueries] after all other [_onDataCallbacks]
   ///
   /// Automatically appended as an [OnData]
-  FutureOr<void> _maybeRebroadcast(QueryResult? result) =>
-      queryManager.maybeRebroadcastQueries(exclude: this);
+  FutureOr<void> _maybeRebroadcast(QueryResult? result) {
+    if (_onDataCallbacks.isEmpty &&
+        result?.hasException == true &&
+        result?.data == null) {
+      // We don't need to rebroadcast if there was an exception and there was no
+      // data. It's valid GQL to have data _and_ exception. If options.carryForwardDataOnException
+      // are true, this condition may never get hit.
+      // If there are onDataCallbacks, it's possible they modify cache and are
+      // depending on maybeRebroadcastQueries being called.
+      return false;
+    }
+    return queryManager.maybeRebroadcastQueries(exclude: this);
+  }
 
   /// The most recently seen result from this operation's stream
   QueryResult<TParsed>? latestResult;

diff --git a/packages/graphql/lib/src/core/query_options.dart b/packages/graphql/lib/src/core/query_options.dart
@@ -55,6 +55,37 @@ class QueryOptions<TParsed extends Object?> extends BaseOptions<TParsed> {
         onError,
       ];
 
+  /// Generic copyWith for all fields. There are other, more specific options:
+  /// - [copyWithPolicies] and [withFetchMoreOptions]
+  QueryOptions<TParsed> copyWithOptions({
+    DocumentNode? document,
+    String? operationName,
+    Map<String, dynamic>? variables,
+    FetchPolicy? fetchPolicy,
+    ErrorPolicy? errorPolicy,
+    CacheRereadPolicy? cacheRereadPolicy,
+    Object? optimisticResult,
+    Duration? pollInterval,
+    Context? context,
+    ResultParserFn<TParsed>? parserFn,
+    OnQueryComplete? onComplete,
+    OnQueryError? onError,
+  }) =>
+      QueryOptions<TParsed>(
+        document: document ?? this.document,
+        operationName: operationName ?? this.operationName,
+        variables: variables ?? this.variables,
+        fetchPolicy: fetchPolicy ?? this.fetchPolicy,
+        errorPolicy: errorPolicy ?? this.errorPolicy,
+        cacheRereadPolicy: cacheRereadPolicy ?? this.cacheRereadPolicy,
+        optimisticResult: optimisticResult ?? this.optimisticResult,
+        pollInterval: pollInterval ?? this.pollInterval,
+        context: context ?? this.context,
+        parserFn: parserFn ?? this.parserFn,
+        onComplete: onComplete ?? this.onComplete,
+        onError: onError ?? this.onError,
+      );
+
   QueryOptions<TParsed> withFetchMoreOptions(
     FetchMoreOptions fetchMoreOptions,
   ) =>
@@ -190,6 +221,40 @@ class WatchQueryOptions<TParsed extends Object?> extends QueryOptions<TParsed> {
         carryForwardDataOnException,
       ];
 
+  /// Generic copyWith for all fields. There are other, more specific options:
+  /// - [copyWithFetchPolicy], [copyWithVariables], etc
+  WatchQueryOptions<TParsed> copyWith({
+    DocumentNode? document,
+    String? operationName,
+    Map<String, dynamic>? variables,
+    FetchPolicy? fetchPolicy,
+    ErrorPolicy? errorPolicy,
+    CacheRereadPolicy? cacheRereadPolicy,
+    Object? optimisticResult,
+    Duration? pollInterval,
+    bool? fetchResults,
+    bool? carryForwardDataOnException,
+    bool? eagerlyFetchResults,
+    Context? context,
+    ResultParserFn<TParsed>? parserFn,
+  }) =>
+      WatchQueryOptions<TParsed>(
+        document: document ?? this.document,
+        operationName: operationName ?? this.operationName,
+        variables: variables ?? this.variables,
+        fetchPolicy: fetchPolicy ?? this.fetchPolicy,
+        errorPolicy: errorPolicy ?? this.errorPolicy,
+        cacheRereadPolicy: cacheRereadPolicy ?? this.cacheRereadPolicy,
+        optimisticResult: optimisticResult ?? this.optimisticResult,
+        pollInterval: pollInterval ?? this.pollInterval,
+        fetchResults: fetchResults ?? this.fetchResults,
+        eagerlyFetchResults: eagerlyFetchResults ?? this.eagerlyFetchResults,
+        carryForwardDataOnException:
+            carryForwardDataOnException ?? this.carryForwardDataOnException,
+        context: context ?? this.context,
+        parserFn: parserFn ?? this.parserFn,
+      );
+
   WatchQueryOptions<TParsed> copyWithFetchPolicy(
     FetchPolicy? fetchPolicy,
   ) =>