From 3291a7627a18120d1ffbbec2a419863c30a0d473 Mon Sep 17 00:00:00 2001 From: Kyle Venn Date: Mon, 22 Jul 2024 22:54:15 -0400 Subject: [PATCH 1/5] Reuse cached query results --- .../lib/src/core/observable_query.dart | 7 +- .../graphql/lib/src/core/query_manager.dart | 100 ++++++++++++++---- .../graphql/lib/src/core/query_options.dart | 7 +- .../graphql/lib/src/core/query_result.dart | 21 +++- 4 files changed, 107 insertions(+), 28 deletions(-) diff --git a/packages/graphql/lib/src/core/observable_query.dart b/packages/graphql/lib/src/core/observable_query.dart index 17d2b7f53..c196d3ef6 100644 --- a/packages/graphql/lib/src/core/observable_query.dart +++ b/packages/graphql/lib/src/core/observable_query.dart @@ -1,5 +1,7 @@ import 'dart:async'; +import 'package:gql/ast.dart'; import 'package:graphql/client.dart'; +import 'package:graphql/src/core/_base_options.dart'; import 'package:meta/meta.dart'; import 'package:graphql/src/core/fetch_more.dart'; @@ -242,12 +244,13 @@ class ObservableQuery { } /// Add a [result] to the [stream] unless it was created - /// before [lasestResult]. + /// before [latestResult]. /// /// Copies the [QueryResult.source] from the [latestResult] /// if it is set to `null`. /// - /// Called internally by the [QueryManager] + /// Called internally by the [QueryManager]. Do not call this directly except + /// for [QueryResult.loading] void addResult(QueryResult result, {bool fromRebroadcast = false}) { // don't overwrite results due to some async/optimism issue if (latestResult != null && diff --git a/packages/graphql/lib/src/core/query_manager.dart b/packages/graphql/lib/src/core/query_manager.dart index 4b4877121..f7ee8f5eb 100644 --- a/packages/graphql/lib/src/core/query_manager.dart +++ b/packages/graphql/lib/src/core/query_manager.dart @@ -1,5 +1,6 @@ import 'dart:async'; +import 'package:gql/ast.dart'; import 'package:graphql/src/utilities/response.dart'; import 'package:meta/meta.dart'; import 'package:collection/collection.dart'; @@ -311,14 +312,26 @@ class QueryManager { // we attempt to resolve the from the cache if (shouldRespondEagerlyFromCache(options.fetchPolicy) && !queryResult.isOptimistic) { - final data = cache.readQuery(request, optimistic: false); - // we only push an eager query with data - if (data != null) { - queryResult = QueryResult( - options: options, - data: data, + final latestResult = _getQueryResultByRequest(request); + if (latestResult != null && latestResult.data != null) { + // we have a result already cached + deserialized for this request + // so we reuse it. + // latest result won't be for loading, it must contain data + queryResult = latestResult.copyWith( source: QueryResultSource.cache, ); + } else { + // otherwise, we try to find the query in cache (which will require + // deserialization) + final data = cache.readQuery(request, optimistic: false); + // we only push an eager query with data + if (data != null) { + queryResult = QueryResult( + options: options, + data: data, + source: QueryResultSource.cache, + ); + } } if (options.fetchPolicy == FetchPolicy.cacheOnly && @@ -358,6 +371,18 @@ class QueryManager { return queryResult; } + /// If a request already has a result associated with it in cache (as + /// determined by [ObservableQuery.latestResult]), we can return it without + /// needing to denormalize + parse again. + QueryResult? _getQueryResultByRequest(Request request) { + for (final query in queries.values) { + if (query.options.asRequest == request) { + return query.latestResult as QueryResult?; + } + } + return null; + } + /// Refetch the [ObservableQuery] referenced by [queryId], /// overriding any present non-network-only [FetchPolicy]. Future?> refetchQuery(String queryId) { @@ -383,11 +408,11 @@ class QueryManager { return results; } - ObservableQuery? getQuery(String? queryId) { - if (!queries.containsKey(queryId)) { + ObservableQuery? getQuery(final String? queryId) { + if (!queries.containsKey(queryId) || queryId == null) { return null; } - final query = queries[queryId!]; + final query = queries[queryId]; if (query is ObservableQuery) { return query; } @@ -402,16 +427,17 @@ class QueryManager { void addQueryResult( Request request, String? queryId, - QueryResult queryResult, - ) { + QueryResult queryResult, { + bool fromRebroadcast = false, + }) { final observableQuery = getQuery(queryId); if (observableQuery != null && !observableQuery.controller.isClosed) { - observableQuery.addResult(queryResult); + observableQuery.addResult(queryResult, fromRebroadcast: fromRebroadcast); } } - /// Create an optimstic result for the query specified by `queryId`, if it exists + /// Create an optimistic result for the query specified by `queryId`, if it exists QueryResult _getOptimisticQueryResult( Request request, { required String queryId, @@ -463,27 +489,55 @@ class QueryManager { return false; } - final shouldBroadast = cache.shouldBroadcast(claimExecution: true); + final shouldBroadcast = cache.shouldBroadcast(claimExecution: true); - if (!shouldBroadast && !force) { + if (!shouldBroadcast && !force) { return false; } - for (var query in queries.values) { - if (query != exclude && query.isRebroadcastSafe) { + // If two ObservableQueries are backed by the same [Request], we only need + // to [readQuery] for it once. + final Map> diffQueryResultCache = {}; + final Map ignoreQueryResults = {}; + for (final query in queries.values) { + final Request request = query.options.asRequest; + final cachedQueryResult = diffQueryResultCache[request]; + if (query == exclude || !query.isRebroadcastSafe) { + continue; + } + if (cachedQueryResult != null) { + // We've already done the diff and denormalized, emit to the observable + addQueryResult( + request, + query.queryId, + cachedQueryResult, + fromRebroadcast: true, + ); + } else if (ignoreQueryResults.containsKey(request)) { + // We've already seen this one and don't need to notify + continue; + } else { + // We haven't seen this one yet, denormalize from cache and diff final cachedData = cache.readQuery( query.options.asRequest, optimistic: query.options.policies.mergeOptimisticData, ); if (_cachedDataHasChangedFor(query, cachedData)) { - query.addResult( - mapFetchResultToQueryResult( - Response(data: cachedData, response: {}), - query.options, - source: QueryResultSource.cache, - ), + // The data has changed + final queryResult = QueryResult( + data: cachedData, + options: query.options, + source: QueryResultSource.cache, + ); + diffQueryResultCache[request] = queryResult; + addQueryResult( + request, + query.queryId, + queryResult, fromRebroadcast: true, ); + } else { + ignoreQueryResults[request] = true; } } } diff --git a/packages/graphql/lib/src/core/query_options.dart b/packages/graphql/lib/src/core/query_options.dart index 529b06219..0ddc639b7 100644 --- a/packages/graphql/lib/src/core/query_options.dart +++ b/packages/graphql/lib/src/core/query_options.dart @@ -168,10 +168,13 @@ class WatchQueryOptions extends QueryOptions { parserFn: parserFn, ); - /// Whether or not to fetch results + /// Whether or not to fetch results every time a new listener is added. + /// If [eagerlyFetchResults] is `true`, fetch is triggered during instantiation. final bool fetchResults; - /// Whether to [fetchResults] immediately on instantiation. + /// Whether to [fetchResults] immediately on instantiation of [ObservableQuery]. + /// The first + /// If available, cache results are emitted when the first listener is added. /// Defaults to [fetchResults]. final bool eagerlyFetchResults; diff --git a/packages/graphql/lib/src/core/query_result.dart b/packages/graphql/lib/src/core/query_result.dart index 5b5b5b38f..da4922ca2 100644 --- a/packages/graphql/lib/src/core/query_result.dart +++ b/packages/graphql/lib/src/core/query_result.dart @@ -48,7 +48,9 @@ class QueryResult { this.context = const Context(), required this.parserFn, required this.source, - }) : timestamp = DateTime.now() { + TParsed? cachedParsedData, + }) : timestamp = DateTime.now(), + _cachedParsedData = cachedParsedData { _data = data; } @@ -160,6 +162,23 @@ class QueryResult { return _cachedParsedData = parserFn(data); } + QueryResult copyWith({ + Map? data, + OperationException? exception, + Context? context, + QueryResultSource? source, + TParsed? cachedParsedData, + }) { + return QueryResult.internal( + data: data ?? this.data, + exception: exception ?? this.exception, + context: context ?? this.context, + parserFn: parserFn, + source: source ?? this.source, + cachedParsedData: cachedParsedData ?? _cachedParsedData, + ); + } + @override String toString() => 'QueryResult(' 'source: $source, ' From 81c4f651aa7e5faa0de1b68c2d976e3739b589df Mon Sep 17 00:00:00 2001 From: Kyle Venn Date: Fri, 2 Aug 2024 17:55:12 -0400 Subject: [PATCH 2/5] Update docs --- packages/graphql/lib/src/core/query_options.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/graphql/lib/src/core/query_options.dart b/packages/graphql/lib/src/core/query_options.dart index 0ddc639b7..1d97ca25a 100644 --- a/packages/graphql/lib/src/core/query_options.dart +++ b/packages/graphql/lib/src/core/query_options.dart @@ -173,8 +173,8 @@ class WatchQueryOptions extends QueryOptions { final bool fetchResults; /// Whether to [fetchResults] immediately on instantiation of [ObservableQuery]. - /// The first /// If available, cache results are emitted when the first listener is added. + /// Network results are then emitted when they return to any attached listeners. /// Defaults to [fetchResults]. final bool eagerlyFetchResults; From 20f624728e16cf8652db0e98cb36c58424924c28 Mon Sep 17 00:00:00 2001 From: Kyle Venn Date: Fri, 2 Aug 2024 22:44:25 -0400 Subject: [PATCH 3/5] Don't rebroadcast for failures --- packages/graphql/lib/src/cache/cache.dart | 16 ++++- .../lib/src/core/observable_query.dart | 15 ++++- .../graphql/lib/src/core/query_options.dart | 65 +++++++++++++++++++ 3 files changed, 93 insertions(+), 3 deletions(-) diff --git a/packages/graphql/lib/src/cache/cache.dart b/packages/graphql/lib/src/cache/cache.dart index ff5cf169c..f69659fc2 100644 --- 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 index c196d3ef6..3be83d572 100644 --- a/packages/graphql/lib/src/core/observable_query.dart +++ b/packages/graphql/lib/src/core/observable_query.dart @@ -95,8 +95,19 @@ class ObservableQuery { /// call [queryManager.maybeRebroadcastQueries] after all other [_onDataCallbacks] /// /// Automatically appended as an [OnData] - FutureOr _maybeRebroadcast(QueryResult? result) => - queryManager.maybeRebroadcastQueries(exclude: this); + FutureOr _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? latestResult; diff --git a/packages/graphql/lib/src/core/query_options.dart b/packages/graphql/lib/src/core/query_options.dart index 1d97ca25a..7a4a43b28 100644 --- a/packages/graphql/lib/src/core/query_options.dart +++ b/packages/graphql/lib/src/core/query_options.dart @@ -55,6 +55,37 @@ class QueryOptions extends BaseOptions { onError, ]; + /// Generic copyWith for all fields. There are other, more specific options: + /// - [copyWithPolicies] and [withFetchMoreOptions] + QueryOptions copyWithOptions({ + DocumentNode? document, + String? operationName, + Map? variables, + FetchPolicy? fetchPolicy, + ErrorPolicy? errorPolicy, + CacheRereadPolicy? cacheRereadPolicy, + Object? optimisticResult, + Duration? pollInterval, + Context? context, + ResultParserFn? parserFn, + OnQueryComplete? onComplete, + OnQueryError? onError, + }) => + QueryOptions( + 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 withFetchMoreOptions( FetchMoreOptions fetchMoreOptions, ) => @@ -190,6 +221,40 @@ class WatchQueryOptions extends QueryOptions { carryForwardDataOnException, ]; + /// Generic copyWith for all fields. There are other, more specific options: + /// - [copyWithFetchPolicy], [copyWithVariables], etc + WatchQueryOptions copyWith({ + DocumentNode? document, + String? operationName, + Map? variables, + FetchPolicy? fetchPolicy, + ErrorPolicy? errorPolicy, + CacheRereadPolicy? cacheRereadPolicy, + Object? optimisticResult, + Duration? pollInterval, + bool? fetchResults, + bool? carryForwardDataOnException, + bool? eagerlyFetchResults, + Context? context, + ResultParserFn? parserFn, + }) => + WatchQueryOptions( + 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 copyWithFetchPolicy( FetchPolicy? fetchPolicy, ) => From 53e324cd8c737a124b3150693b753a93f1fcf5c6 Mon Sep 17 00:00:00 2001 From: Kyle Venn Date: Sun, 4 Aug 2024 18:08:38 -0400 Subject: [PATCH 4/5] Remove unused imports --- packages/graphql/lib/src/core/observable_query.dart | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/graphql/lib/src/core/observable_query.dart b/packages/graphql/lib/src/core/observable_query.dart index 3be83d572..99d0ba612 100644 --- a/packages/graphql/lib/src/core/observable_query.dart +++ b/packages/graphql/lib/src/core/observable_query.dart @@ -1,7 +1,5 @@ import 'dart:async'; -import 'package:gql/ast.dart'; import 'package:graphql/client.dart'; -import 'package:graphql/src/core/_base_options.dart'; import 'package:meta/meta.dart'; import 'package:graphql/src/core/fetch_more.dart'; From de15f684b1fb0fd3473ad663e2e1b0a57150a49e Mon Sep 17 00:00:00 2001 From: Kyle Venn Date: Sun, 4 Aug 2024 18:11:15 -0400 Subject: [PATCH 5/5] Remove unused import --- packages/graphql/lib/src/core/query_manager.dart | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/graphql/lib/src/core/query_manager.dart b/packages/graphql/lib/src/core/query_manager.dart index f7ee8f5eb..a74578c96 100644 --- a/packages/graphql/lib/src/core/query_manager.dart +++ b/packages/graphql/lib/src/core/query_manager.dart @@ -1,6 +1,5 @@ import 'dart:async'; -import 'package:gql/ast.dart'; import 'package:graphql/src/utilities/response.dart'; import 'package:meta/meta.dart'; import 'package:collection/collection.dart';