Skip to content

Commit

Permalink
fix(graphcache): Improve separation of API and owned data (#3165)
Browse files Browse the repository at this point in the history
  • Loading branch information
kitten authored Apr 20, 2023
1 parent b5e50b4 commit 6031d91
Show file tree
Hide file tree
Showing 20 changed files with 165 additions and 146 deletions.
5 changes: 5 additions & 0 deletions .changeset/gentle-melons-march.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@urql/exchange-graphcache': patch
---

Prevent reusal of incoming API data in Graphcache’s produced (“owned”) data. This prevents us from copying the `__typename` and other superfluous fields.
22 changes: 3 additions & 19 deletions exchanges/graphcache/src/cacheExchange.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,17 +100,12 @@ describe('data dependencies', () => {
expect(response).toHaveBeenCalledTimes(1);
expect(result).toHaveBeenCalledTimes(2);

expect(result.mock.calls[0][0]).toHaveProperty(
'operation.context.meta.cacheOutcome',
'miss'
);
expect(result.mock.calls[0][0].data).toEqual(expected);
expect(expected).toMatchObject(result.mock.calls[0][0].data);
expect(result.mock.calls[1][0]).toHaveProperty(
'operation.context.meta.cacheOutcome',
'hit'
);
expect(result.mock.calls[1][0].data).toEqual(expected);

expect(expected).toMatchObject(result.mock.calls[1][0].data);
expect(result.mock.calls[1][0].data).toBe(result.mock.calls[0][0].data);
});

Expand Down Expand Up @@ -230,7 +225,7 @@ describe('data dependencies', () => {

next(opMultiple);
expect(response).toHaveBeenCalledTimes(2);
expect(reexec).toHaveBeenCalledWith(opOne);
expect(reexec.mock.calls[0][0]).toHaveProperty('key', opOne.key);
expect(result).toHaveBeenCalledTimes(3);

// test for reference reuse
Expand Down Expand Up @@ -775,7 +770,6 @@ describe('optimistic updates', () => {
expect(reexec).toHaveBeenCalledTimes(1);

expect(result.mock.calls[1][0]?.data).toMatchObject({
__typename: 'Query',
author: { name: '[REDACTED OFFLINE]' },
});

Expand Down Expand Up @@ -1263,7 +1257,6 @@ describe('custom resolvers', () => {

vi.runAllTimers();
expect(result.mock.calls[1][0].data).toEqual({
__typename: 'Mutation',
concealAuthor: {
__typename: 'Author',
id: '123',
Expand Down Expand Up @@ -1429,7 +1422,6 @@ describe('custom resolvers', () => {
expect(response).toHaveBeenCalledTimes(2);
expect(fakeResolver).toHaveBeenCalledTimes(6);
expect(result.mock.calls[1][0].data).toEqual({
__typename: 'Mutation',
concealAuthors: [
{
__typename: 'Author',
Expand Down Expand Up @@ -1583,10 +1575,6 @@ describe('schema awareness', () => {
],
});

expect(result.mock.calls[0][0]).not.toHaveProperty(
'operation.context.meta'
);

next(queryOperation);
vi.runAllTimers();
expect(result).toHaveBeenCalledTimes(2);
Expand Down Expand Up @@ -1725,10 +1713,6 @@ describe('schema awareness', () => {
],
});

expect(result.mock.calls[0][0]).not.toHaveProperty(
'operation.context.meta'
);

next(queryOperation);
vi.runAllTimers();
expect(result).toHaveBeenCalledTimes(2);
Expand Down
85 changes: 47 additions & 38 deletions exchanges/graphcache/src/cacheExchange.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,18 @@ import {
Source,
} from 'wonka';

import { query, write, writeOptimistic } from './operations';
import { _query } from './operations/query';
import { _write } from './operations/write';
import { addMetadata, toRequestPolicy } from './helpers/operation';
import { filterVariables, getMainOperation } from './ast';
import { Store, noopDataState, hydrateData, reserveLayer } from './store';
import {
Store,
initDataState,
clearDataState,
noopDataState,
hydrateData,
reserveLayer,
} from './store';
import { Data, Dependencies, CacheExchangeOpts } from './types';

interface OperationResultWithMeta extends Partial<OperationResult> {
Expand Down Expand Up @@ -110,7 +118,6 @@ export const cacheExchange =
if (op) {
// Collect all dependent operations if the reexecuting operation is a query
if (operation.kind === 'query') dependentOperations.add(key);
operations.delete(key);
let policy: RequestPolicy = 'cache-first';
if (requestedRefetch.has(key)) {
requestedRefetch.delete(key);
Expand All @@ -135,6 +142,7 @@ export const cacheExchange =
if (operation.kind === 'query') {
// Pre-reserve the position of the result layer
reserveLayer(store.data, operation.key);
operations.set(operation.key, operation);
} else if (operation.kind === 'teardown') {
// Delete reference to operation if any exists to release it
operations.delete(operation.key);
Expand All @@ -146,12 +154,11 @@ export const cacheExchange =
operation.kind === 'mutation' &&
operation.context.requestPolicy !== 'network-only'
) {
operations.set(operation.key, operation);
// This executes an optimistic update for mutations and registers it if necessary
const { dependencies } = writeOptimistic(
store,
operation,
operation.key
);
initDataState('write', store.data, operation.key, true, false);
const { dependencies } = _write(store, operation, undefined, undefined);
clearDataState();
if (dependencies.size) {
// Update blocked optimistic dependencies
for (const dep of dependencies.values()) blockedDependencies.add(dep);
Expand All @@ -178,7 +185,7 @@ export const cacheExchange =
)
: operation.variables,
},
{ ...operation.context, originalVariables: operation.variables }
operation.context
);
};

Expand All @@ -196,15 +203,21 @@ export const cacheExchange =
const operationResultFromCache = (
operation: Operation
): OperationResultWithMeta => {
const result = query(store, operation, results.get(operation.key));
initDataState('read', store.data, undefined, false, false);
const result = _query(
store,
operation,
results.get(operation.key),
undefined
);
clearDataState();
const cacheOutcome: CacheOutcome = result.data
? !result.partial && !result.hasNext
? 'hit'
: 'partial'
: 'miss';

results.set(operation.key, result.data);
operations.set(operation.key, operation);
updateDependencies(operation, result.dependencies);

return {
Expand All @@ -222,21 +235,9 @@ export const cacheExchange =
pendingOperations: Operations
): OperationResult => {
// Retrieve the original operation to remove changes made by formatDocument
const originalOperation = operations.get(result.operation.key);
const operation = originalOperation
? makeOperation(
originalOperation.kind,
originalOperation,
result.operation.context
)
: result.operation;

const operation =
operations.get(result.operation.key) || result.operation;
if (operation.kind === 'mutation') {
if (result.operation.context.originalVariables) {
operation.variables = result.operation.context.originalVariables;
delete result.operation.context.originalVariables;
}

// Collect previous dependencies that have been written for optimistic updates
const dependencies = optimisticKeysToDependencies.get(operation.key);
collectPendingOperations(pendingOperations, dependencies);
Expand All @@ -251,25 +252,31 @@ export const cacheExchange =
if (data) {
// Write the result to cache and collect all dependencies that need to be
// updated
const writeDependencies = write(
initDataState('write', store.data, operation.key, false, false);
const writeDependencies = _write(
store,
operation,
data,
result.error,
operation.key
result.error
).dependencies;
clearDataState();
collectPendingOperations(pendingOperations, writeDependencies);

const queryResult = query(
const prevData =
operation.kind === 'query' ? results.get(operation.key) : null;
initDataState(
'read',
store.data,
operation.key,
false,
prevData !== data
);
const queryResult = _query(
store,
operation,
operation.kind === 'query'
? results.get(operation.key) || data
: data,
result.error,
operation.key
prevData || data,
result.error
);

clearDataState();
data = queryResult.data;
if (operation.kind === 'query') {
// Collect the query's dependencies for future pending operation updates
Expand All @@ -283,7 +290,6 @@ export const cacheExchange =

// Update this operation's dependencies if it's a query
if (queryDependencies) {
operations.set(operation.key, operation);
updateDependencies(result.operation, queryDependencies);
}

Expand Down Expand Up @@ -374,7 +380,10 @@ export const cacheExchange =
/*noop*/
} else if (!isBlockedByOptimisticUpdate(res.dependencies)) {
client.reexecuteOperation(
toRequestPolicy(res.operation, 'network-only')
toRequestPolicy(
operations.get(res.operation.key) || res.operation,
'network-only'
)
);
} else if (requestPolicy === 'cache-and-network') {
requestedRefetch.add(res.operation.key);
Expand Down
3 changes: 2 additions & 1 deletion exchanges/graphcache/src/extras/relayPagination.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { gql } from '@urql/core';
import { it, expect } from 'vitest';
import { query, write } from '../operations';
import { __initAnd_query as query } from '../operations/query';
import { __initAnd_write as write } from '../operations/write';
import { Store } from '../store';
import { relayPagination } from './relayPagination';

Expand Down
3 changes: 2 additions & 1 deletion exchanges/graphcache/src/extras/simplePagination.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { gql } from '@urql/core';
import { it, expect } from 'vitest';
import { query, write } from '../operations';
import { __initAnd_query as query } from '../operations/query';
import { __initAnd_write as write } from '../operations/write';
import { Store } from '../store';
import { simplePagination } from './simplePagination';

Expand Down
3 changes: 1 addition & 2 deletions exchanges/graphcache/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
export * from './types';
export { query, write } from './operations';
export { Store } from './store';
export type { Store } from './store';
export { cacheExchange } from './cacheExchange';
export { offlineExchange } from './offlineExchange';
2 changes: 1 addition & 1 deletion exchanges/graphcache/src/offlineExchange.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ describe('offline', () => {

next(queryOp);
expect(result).toBeCalledTimes(1);
expect(result.mock.calls[0][0].data).toMatchObject(queryOneData);
expect(queryOneData).toMatchObject(result.mock.calls[0][0].data);

next(mutationOp);
expect(result).toBeCalledTimes(1);
Expand Down
2 changes: 0 additions & 2 deletions exchanges/graphcache/src/operations/index.ts

This file was deleted.

51 changes: 26 additions & 25 deletions exchanges/graphcache/src/operations/query.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import { minifyIntrospectionQuery } from '@urql/introspection';
import { describe, it, beforeEach, beforeAll, expect } from 'vitest';

import { Store } from '../store';
import { write } from './write';
import { query } from './query';
import { __initAnd_write as write } from './write';
import { __initAnd_query as query } from './query';

const TODO_QUERY = gql`
query Todos {
Expand Down Expand Up @@ -363,7 +363,7 @@ describe('Query', () => {
expect(previousData).toHaveProperty('todos.0.textB', 'old');
});

it('should not keep references stable', () => {
it('should keep references stable', () => {
const QUERY = gql`
query todos {
__typename
Expand Down Expand Up @@ -398,31 +398,32 @@ describe('Query', () => {

write(store, { query: QUERY }, expected);

const prevData = {
todos: [
{
__typename: 'Todo',
id: 'prev-0',
},
{
__typename: 'Todo',
id: '1',
},
{
__typename: 'Todo',
id: '2',
},
],
__typename: 'query_root',
};
const prevData = query(
store,
{ query: QUERY },
{
todos: [
{
__typename: 'Todo',
id: 'prev-0',
},
{
__typename: 'Todo',
id: '1',
},
{
__typename: 'Todo',
id: '2',
},
],
__typename: 'query_root',
}
).data as any;

const data = query(store, { query: QUERY }, prevData)
.data as typeof expected;
const data = query(store, { query: QUERY }, prevData).data as any;
expect(data).toEqual(expected);

expect(prevData.todos[0]).not.toEqual(data.todos[0]);
expect(prevData.todos[0]).not.toBe(data.todos[0]);
// unchanged references:
expect(prevData.todos[0]).toBe(data.todos[0]);
expect(prevData.todos[1]).toBe(data.todos[1]);
expect(prevData.todos[2]).toBe(data.todos[2]);
});
Expand Down
Loading

0 comments on commit 6031d91

Please sign in to comment.