Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a @nonreactive directive for selectively skipping reactive comparisons of query result subtrees #10722

Merged
merged 18 commits into from
Apr 5, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/modern-peaches-marry.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@apollo/client': minor
---

Implement a `@nonreactive` directive for selectively skipping reactive comparisons of query result subtrees
3 changes: 2 additions & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@
"files": ["**/__tests__/**/*.[jt]sx", "**/?(*.)+(test).[jt]sx"],
"extends": ["plugin:testing-library/react"],
"rules": {
"testing-library/prefer-user-event": "error"
"testing-library/prefer-user-event": "error",
"testing-library/no-wait-for-multiple-assertions": "off"
}
}
]
Expand Down
2 changes: 1 addition & 1 deletion config/bundlesize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { join } from "path";
import { gzipSync } from "zlib";
import bytes from "bytes";

const gzipBundleByteLengthLimit = bytes("34.14KB");
const gzipBundleByteLengthLimit = bytes("34.5KB");
const minFile = join("dist", "apollo-client.min.cjs");
const minPath = join(__dirname, "..", minFile);
const gzipByteLen = gzipSync(readFileSync(minPath)).byteLength;
Expand Down
49 changes: 34 additions & 15 deletions src/core/ObservableQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {
Observer,
ObservableSubscription,
iterateObserversSafely,
isNonEmptyArray,
fixObservableSubclass,
getQueryDefinition,
} from '../utilities';
Expand All @@ -32,6 +31,7 @@ import {
import { QueryInfo } from './QueryInfo';
import { MissingFieldError } from '../cache';
import { MissingTree } from '../cache/core/types/common';
import { compareResultsUsingQuery } from './compareResults';

const {
assign,
Expand Down Expand Up @@ -307,9 +307,23 @@ export class ObservableQuery<
newResult: ApolloQueryResult<TData>,
variables?: TVariables
) {
if (!this.last) {
return true;
}

const { query } = this.options;
const resultIsDifferent =
this.queryManager.transform(query).hasNonreactiveDirective
? !compareResultsUsingQuery(
query,
this.last.result,
newResult,
this.variables,
)
: !equal(this.last.result, newResult);
Copy link
Member Author

@benjamn benjamn Apr 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we handle the backwards compatibility logic of using compareResultsUsingQuery only when the query contains one or more @nonreactive directives.

Despite appearances, this.queryManager.transform(query).hasNonreactiveDirective is cached after the first time query is encountered, so we do not have to keep scanning the query for @nonreactive directives every time this code runs.


return (
!this.last ||
!equal(this.last.result, newResult) ||
resultIsDifferent ||
(variables && !equal(this.last.variables, variables))
);
}
Expand Down Expand Up @@ -769,17 +783,18 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`);
newResult: ApolloQueryResult<TData>,
variables = this.variables,
) {
this.last = {
...this.last,
let error: ApolloError | undefined = this.getLastError();
// Preserve this.last.error unless the variables have changed.
if (error && this.last && !equal(variables, this.last.variables)) {
error = void 0;
}
return this.last = {
result: this.queryManager.assumeImmutableResults
? newResult
: cloneDeep(newResult),
variables,
...(error ? { error } : null),
};
if (!isNonEmptyArray(newResult.errors)) {
delete this.last.error;
}
return this.last;
}

public reobserve(
Expand Down Expand Up @@ -879,12 +894,16 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`);
variables: TVariables | undefined,
) {
const lastError = this.getLastError();
if (lastError || this.isDifferentFromLastResult(result, variables)) {
if (lastError || !result.partial || this.options.returnPartialData) {
this.updateLastResult(result, variables);
}

iterateObserversSafely(this.observers, 'next', result);
const isDifferent = this.isDifferentFromLastResult(result, variables);
// Update the last result even when isDifferentFromLastResult returns false,
// because the query may be using the @nonreactive directive, and we want to
// save the the latest version of any nonreactive subtrees (in case
// getCurrentResult is called), even though we skip broadcasting changes.
if (lastError || !result.partial || this.options.returnPartialData) {
this.updateLastResult(result, variables);
}
if (lastError || isDifferent) {
iterateObserversSafely(this.observers, "next", result);
}
}

Expand Down
21 changes: 14 additions & 7 deletions src/core/QueryManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@ import { equal } from '@wry/equality';

import { ApolloLink, execute, FetchResult } from '../link/core';
import {
hasDirectives,
isExecutionPatchIncrementalResult,
isExecutionPatchResult,
removeDirectivesFromDocument,
} from '../utilities';
import { Cache, ApolloCache, canonicalStringify } from '../cache';

Expand All @@ -19,7 +21,6 @@ import {
hasClientExports,
graphQLResultHasError,
getGraphQLErrorsFromResult,
removeConnectionDirectiveFromDocument,
canUseWeakMap,
ObservableSubscription,
Observable,
Expand Down Expand Up @@ -77,6 +78,7 @@ interface TransformCacheEntry {
document: DocumentNode;
hasClientExports: boolean;
hasForcedResolvers: boolean;
hasNonreactiveDirective: boolean;
clientQuery: DocumentNode | null;
serverQuery: DocumentNode | null;
defaultVars: OperationVariables;
Expand Down Expand Up @@ -616,18 +618,23 @@ export class QueryManager<TStore> {

if (!transformCache.has(document)) {
const transformed = this.cache.transformDocument(document);
const noConnection = removeConnectionDirectiveFromDocument(transformed);
const serverQuery = removeDirectivesFromDocument([
removeClientFields ? { name: 'client', remove: true } : {},
{ name: 'connection' },
{ name: 'nonreactive' },
], transformed);
Comment on lines +621 to +625
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like @client and @connection, the @nonreactive directive is removed from queries before they are sent to the server, though (unlike @client) only the @nonreactive directive is removed, preserving the field/fragment it was annotating (because we want all the data from the server, even if the client chooses not to broadcast it).

Thanks to 3030094, all three of these removals can be performed in the same traversal of the AST!

const clientQuery = this.localState.clientQuery(transformed);
const serverQuery =
noConnection &&
this.localState.serverQuery(noConnection, { removeClientFields });

const cacheEntry: TransformCacheEntry = {
document: transformed,
// TODO These two calls (hasClientExports and shouldForceResolvers)
// could probably be merged into a single traversal.
// TODO These three calls (hasClientExports, shouldForceResolvers, and
// usesNonreactiveDirective) are performing independent full traversals
// of the transformed document. We should consider merging these
// traversals into a single pass in the future, though the work is
// cached after the first time.
hasClientExports: hasClientExports(transformed),
hasForcedResolvers: this.localState.shouldForceResolvers(transformed),
hasNonreactiveDirective: hasDirectives(['nonreactive'], transformed),
clientQuery,
serverQuery,
defaultVars: getDefaultValues(
Expand Down