From 37d7e798f53476bb1c814a31dd02114066347ecf Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Mon, 16 Aug 2021 23:48:23 +0100 Subject: [PATCH] (graphcache) - Increase structural (deep) equality in normalized query results (#1859) * Refactor dataFieldValue assignment in readSelection * Add initial reusal of data logic * Add data reusal logic to readRoot * Distribute input data throughout query operation * Prevent null from being carried over from previous results * Replace WeakSet with Set for ownership tracking * Add test for separate and shared references * Incrementally query cached data * Reuse last result rather than API result for queries * Add on reference reuse test to cacheExchange test * Add changeset * Reduce performance impact by writing eagerly --- .changeset/eleven-cobras-flash.md | 7 + .../graphcache/default-storage/package.json | 2 +- .../graphcache/src/cacheExchange.test.ts | 20 +- exchanges/graphcache/src/cacheExchange.ts | 48 ++-- .../graphcache/src/offlineExchange.test.ts | 7 +- .../graphcache/src/operations/query.test.ts | 243 ++++++++++++------ exchanges/graphcache/src/operations/query.ts | 171 ++++++------ exchanges/graphcache/src/operations/shared.ts | 2 +- exchanges/graphcache/src/store/data.ts | 25 ++ exchanges/graphcache/src/store/index.ts | 2 + 10 files changed, 334 insertions(+), 193 deletions(-) create mode 100644 .changeset/eleven-cobras-flash.md diff --git a/.changeset/eleven-cobras-flash.md b/.changeset/eleven-cobras-flash.md new file mode 100644 index 0000000000..11cd776abc --- /dev/null +++ b/.changeset/eleven-cobras-flash.md @@ -0,0 +1,7 @@ +--- +'@urql/exchange-graphcache': minor +--- + +Improve referential equality of deeply queried objects from the normalised cache for queries. Each query operation will now reuse the last known result and only incrementally change references as necessary, scanning over the previous result to identify whether anything has changed. + +While this affects our querying performance by about -20%, it should yield noticeable results in UI frameworks, where referential equality is often used to avoid work (e.g. in React with `useMemo` or `React.memo`). Overall, a 20% difference shouldn't be noticeable in day to day use given that we can read fairly typical queries over 10K times a second. ⚡ diff --git a/exchanges/graphcache/default-storage/package.json b/exchanges/graphcache/default-storage/package.json index 5f6d001186..9067571b37 100644 --- a/exchanges/graphcache/default-storage/package.json +++ b/exchanges/graphcache/default-storage/package.json @@ -16,7 +16,7 @@ "./package.json": "./package.json" }, "dependencies": { - "@urql/core": ">=2.0.0", + "@urql/core": ">=2.1.5", "wonka": "^4.0.14" } } diff --git a/exchanges/graphcache/src/cacheExchange.test.ts b/exchanges/graphcache/src/cacheExchange.test.ts index 48a4ffc54d..88103ca00b 100644 --- a/exchanges/graphcache/src/cacheExchange.test.ts +++ b/exchanges/graphcache/src/cacheExchange.test.ts @@ -30,6 +30,9 @@ const queryOne = gql` id name } + unrelated { + id + } } `; @@ -40,6 +43,10 @@ const queryOneData = { id: '123', name: 'Author', }, + unrelated: { + __typename: 'Unrelated', + id: 'unrelated', + }, }; const dispatchDebug = jest.fn(); @@ -142,7 +149,7 @@ describe('data dependencies', () => { { __typename: 'Author', id: '123', - name: 'Author', + name: 'New Author Name', }, ], }; @@ -193,6 +200,13 @@ describe('data dependencies', () => { expect(response).toHaveBeenCalledTimes(2); expect(reexec).toHaveBeenCalledWith(opOne); expect(result).toHaveBeenCalledTimes(3); + + // test for reference reuse + const firstDataOne = result.mock.calls[0][0].data; + const firstDataTwo = result.mock.calls[1][0].data; + expect(firstDataOne).not.toBe(firstDataTwo); + expect(firstDataOne.author).not.toBe(firstDataTwo.author); + expect(firstDataOne.unrelated).toBe(firstDataTwo.unrelated); }); it('updates related queries when a mutation update touches query data', () => { @@ -1125,7 +1139,7 @@ describe('custom resolvers', () => { expect(response).toHaveBeenCalledTimes(1); expect(fakeResolver).toHaveBeenCalledTimes(1); expect(result).toHaveBeenCalledTimes(1); - expect(result.mock.calls[0][0].data).toEqual({ + expect(result.mock.calls[0][0].data).toMatchObject({ author: { id: '123', name: 'newName', @@ -1491,7 +1505,7 @@ describe('schema awareness', () => { jest.runAllTimers(); expect(response).toHaveBeenCalledTimes(1); expect(reexec).toHaveBeenCalledTimes(0); - expect(result.mock.calls[0][0].data).toEqual({ + expect(result.mock.calls[0][0].data).toMatchObject({ todos: [ { __typename: 'Todo', diff --git a/exchanges/graphcache/src/cacheExchange.ts b/exchanges/graphcache/src/cacheExchange.ts index 70de0d164e..36ec383133 100644 --- a/exchanges/graphcache/src/cacheExchange.ts +++ b/exchanges/graphcache/src/cacheExchange.ts @@ -28,7 +28,7 @@ import { makeDict, isDictEmpty } from './helpers/dict'; import { addCacheOutcome, toRequestPolicy } from './helpers/operation'; import { filterVariables, getMainOperation } from './ast'; import { Store, noopDataState, hydrateData, reserveLayer } from './store'; -import { Dependencies, CacheExchangeOpts } from './types'; +import { Data, Dependencies, CacheExchangeOpts } from './types'; type OperationResultWithMeta = OperationResult & { outcome: CacheOutcome; @@ -37,6 +37,7 @@ type OperationResultWithMeta = OperationResult & { type Operations = Set; type OperationMap = Map; +type ResultMap = Map; type OptimisticDependencies = Map; type DependentOperations = Record; @@ -54,7 +55,8 @@ export const cacheExchange = >( const optimisticKeysToDependencies: OptimisticDependencies = new Map(); const mutationResultBuffer: OperationResult[] = []; - const ops: OperationMap = new Map(); + const operations: OperationMap = new Map(); + const results: ResultMap = new Map(); const blockedDependencies: Dependencies = makeDict(); const requestedRefetch: Operations = new Set(); const deps: DependentOperations = makeDict(); @@ -89,9 +91,9 @@ export const cacheExchange = >( // Reexecute collected operations and delete them from the mapping pendingOperations.forEach(key => { if (key !== operation.key) { - const op = ops.get(key); + const op = operations.get(key); if (op) { - ops.delete(key); + operations.delete(key); let policy: RequestPolicy = 'cache-first'; if (requestedRefetch.has(key)) { requestedRefetch.delete(key); @@ -110,7 +112,8 @@ export const cacheExchange = >( reserveLayer(store.data, operation.key); } else if (operation.kind === 'teardown') { // Delete reference to operation if any exists to release it - ops.delete(operation.key); + operations.delete(operation.key); + results.delete(operation.key); // Mark operation layer as done noopDataState(store.data, operation.key); } else if ( @@ -155,7 +158,7 @@ export const cacheExchange = >( const updateDependencies = (op: Operation, dependencies: Dependencies) => { for (const dep in dependencies) { (deps[dep] || (deps[dep] = [])).push(op.key); - ops.set(op.key, op); + operations.set(op.key, op); } }; @@ -164,20 +167,21 @@ export const cacheExchange = >( const operationResultFromCache = ( operation: Operation ): OperationResultWithMeta => { - const res = query(store, operation); - const cacheOutcome: CacheOutcome = res.data - ? !res.partial + const result = query(store, operation, results.get(operation.key)); + const cacheOutcome: CacheOutcome = result.data + ? !result.partial ? 'hit' : 'partial' : 'miss'; - updateDependencies(operation, res.dependencies); + results.set(operation.key, result.data); + updateDependencies(operation, result.dependencies); return { outcome: cacheOutcome, operation, - data: res.data, - dependencies: res.dependencies, + data: result.data, + dependencies: result.dependencies, }; }; @@ -199,30 +203,28 @@ export const cacheExchange = >( } let queryDependencies: void | Dependencies; - if (result.data) { + let data: Data | null = result.data; + if (data) { // Write the result to cache and collect all dependencies that need to be // updated - const writeDependencies = write( - store, - operation, - result.data, - result.error, - key - ).dependencies; + const writeDependencies = write(store, operation, data, result.error, key) + .dependencies; collectPendingOperations(pendingOperations, writeDependencies); const queryResult = query( store, operation, - result.data, + operation.kind === 'query' ? results.get(operation.key) || data : data, result.error, key ); - result.data = queryResult.data; + + data = queryResult.data; if (operation.kind === 'query') { // Collect the query's dependencies for future pending operation updates queryDependencies = queryResult.dependencies; collectPendingOperations(pendingOperations, queryDependencies); + results.set(operation.key, result.data); } } else { noopDataState(store.data, operation.key); @@ -233,7 +235,7 @@ export const cacheExchange = >( updateDependencies(result.operation, queryDependencies); } - return { data: result.data, error, extensions, operation }; + return { data, error, extensions, operation }; }; return ops$ => { diff --git a/exchanges/graphcache/src/offlineExchange.test.ts b/exchanges/graphcache/src/offlineExchange.test.ts index 6c170a03c5..01604af0b7 100644 --- a/exchanges/graphcache/src/offlineExchange.test.ts +++ b/exchanges/graphcache/src/offlineExchange.test.ts @@ -168,10 +168,7 @@ describe('offline', () => { next(queryOp); expect(result).toBeCalledTimes(1); - expect(result.mock.calls[0][0].data).toEqual({ - ...queryOneData, - __typename: undefined, - }); + expect(result.mock.calls[0][0].data).toMatchObject(queryOneData); next(mutationOp); expect(result).toBeCalledTimes(1); @@ -192,7 +189,7 @@ describe('offline', () => { next(queryOp); expect(result).toBeCalledTimes(2); - expect(result.mock.calls[1][0].data).toEqual({ + expect(result.mock.calls[1][0].data).toMatchObject({ authors: [{ id: '123', name: 'URQL', __typename: 'Author' }], }); }); diff --git a/exchanges/graphcache/src/operations/query.test.ts b/exchanges/graphcache/src/operations/query.test.ts index f6524ca39f..20335b3bba 100644 --- a/exchanges/graphcache/src/operations/query.test.ts +++ b/exchanges/graphcache/src/operations/query.test.ts @@ -3,7 +3,6 @@ import { gql } from '@urql/core'; import { minifyIntrospectionQuery } from '@urql/introspection'; -import { Data } from '../types'; import { Store } from '../store'; import { write } from './write'; import { query } from './query'; @@ -131,85 +130,6 @@ describe('Query', () => { expect((console.warn as any).mock.calls[0][0]).toMatch(/writer/); }); - it('should not overwrite different arrays from results and queries', () => { - const TODO_QUERY = gql` - query Todos { - todos { - __typename - node { - __typename - id - text - author { - __typename - id - } - } - } - } - `; - - store = new Store({ - resolvers: { - Query: { - todos: (_parent, _args, cache) => cache.resolve('Query', 'todos'), - }, - }, - }); - - const expected = ({ - todos: [ - { - __typename: 'TodoEdge', - node: { - __typename: 'Todo', - id: '0', - text: 'Teach', - author: { - __typename: 'Author', - id: 'writy-mcwriteface', - }, - }, - }, - { - __typename: 'TodoEdge', - node: { - __typename: 'Todo', - id: '1', - text: 'Learn', - author: null, - }, - }, - ], - } as any) as Data; - - write(store, { query: TODO_QUERY }, expected); - - const result = query( - store, - { query: TODO_QUERY }, - { - __typename: 'Query', - todos: [ - // NOTE: This is a partial list of later results - { - __typename: 'TodoEdge', - node: { - id: '1', - text: 'Learn', - __typename: 'Todo', - author: null, - }, - }, - ], - } - ); - - expect(result.data).toEqual(expected); - expect(result.data).not.toBe(expected); - expect(result.data!.todos![0]).not.toBe(expected!.todos![0]); - }); - // Issue#64 it('should not crash for valid queries', () => { const VALID_QUERY = gql` @@ -284,7 +204,7 @@ describe('Query', () => { expect(console.error).not.toHaveBeenCalled(); }); - it('should allow subsequent read when first result was null', () => { + it('should not allow subsequent reads when first result was null', () => { const QUERY_WRITE = gql` query writeTodos { __typename @@ -312,10 +232,6 @@ describe('Query', () => { } } - fragment ValidRead on Todo { - id - } - fragment MissingRead on Todo { id text @@ -352,4 +268,161 @@ describe('Query', () => { expect(console.warn).not.toHaveBeenCalled(); expect(console.error).not.toHaveBeenCalled(); }); + + it('should not mix references', () => { + const QUERY_WRITE = gql` + query writeTodos { + __typename + todos { + __typename + id + textA + textB + } + } + `; + + const QUERY_READ = gql` + query getTodos { + __typename + todos { + __typename + id + textA + } + todos { + __typename + textB + } + } + `; + + const store = new Store({ + schema: alteredRoot, + }); + + write( + store, + { query: QUERY_WRITE }, + { + todos: [ + { + __typename: 'Todo', + id: '0', + textA: 'a', + textB: 'b', + }, + ], + __typename: 'Query', + } + ); + + let data: any; + data = query(store, { query: QUERY_READ }).data; + + expect(data).toEqual({ + __typename: 'query_root', + todos: [ + { + __typename: 'Todo', + id: '0', + textA: 'a', + textB: 'b', + }, + ], + }); + + const previousData = { + __typename: 'query_root', + todos: [ + { + __typename: 'Todo', + id: '0', + textA: 'a', + textB: 'old', + }, + ], + }; + + data = query(store, { query: QUERY_READ }, previousData).data; + + expect(data).toEqual({ + __typename: 'query_root', + todos: [ + { + __typename: 'Todo', + id: '0', + textA: 'a', + textB: 'b', + }, + ], + }); + + expect(previousData).toHaveProperty('todos.0.textA', 'a'); + expect(previousData).toHaveProperty('todos.0.textB', 'old'); + }); + + it('should not keep references stable', () => { + const QUERY = gql` + query todos { + __typename + todos { + __typename + id + } + } + `; + + const store = new Store({ + schema: alteredRoot, + }); + + const expected = { + todos: [ + { + __typename: 'Todo', + id: '0', + }, + { + __typename: 'Todo', + id: '1', + }, + { + __typename: 'Todo', + id: '2', + }, + ], + __typename: 'query_root', + }; + + 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 data = query(store, { query: QUERY }, prevData) + .data as typeof expected; + 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[1]).toBe(data.todos[1]); + expect(prevData.todos[2]).toBe(data.todos[2]); + }); }); diff --git a/exchanges/graphcache/src/operations/query.ts b/exchanges/graphcache/src/operations/query.ts index 67825e07b7..f7460f63cd 100644 --- a/exchanges/graphcache/src/operations/query.ts +++ b/exchanges/graphcache/src/operations/query.ts @@ -30,6 +30,8 @@ import { clearDataState, joinKeys, keyOfField, + makeData, + ownsData, } from '../store'; import * as InMemoryData from '../store/data'; @@ -59,7 +61,7 @@ export interface QueryResult { export const query = ( store: Store, request: OperationRequest, - data?: Data, + data?: Data | null | undefined, error?: CombinedError | undefined, key?: number ): QueryResult => { @@ -72,7 +74,7 @@ export const query = ( export const read = ( store: Store, request: OperationRequest, - input?: Data, + input?: Data | null | undefined, error?: CombinedError | undefined ): QueryResult => { const operation = getMainOperation(request.query); @@ -93,14 +95,15 @@ export const read = ( pushDebugNode(rootKey, operation); } + if (!input) input = makeData(); // NOTE: This may reuse "previous result data" as indicated by the // `originalData` argument in readRoot(). This behaviour isn't used // for readSelection() however, which always produces results from // scratch const data = rootKey !== ctx.store.rootFields['query'] - ? readRoot(ctx, rootKey, rootSelect, input || ({} as Data)) - : readSelection(ctx, rootKey, rootSelect, {} as Data); + ? readRoot(ctx, rootKey, rootSelect, input) + : readSelection(ctx, rootKey, rootSelect, input); if (process.env.NODE_ENV !== 'production') { popDebugNode(); @@ -117,36 +120,45 @@ const readRoot = ( ctx: Context, entityKey: string, select: SelectionSet, - originalData: Data + data: Data ): Data => { - const typename = ctx.store.rootNames[entityKey] - ? entityKey - : originalData.__typename; + const typename = ctx.store.rootNames[entityKey] ? entityKey : data.__typename; if (typeof typename !== 'string') { - return originalData; + return data; } const iterate = makeSelectionIterator(entityKey, entityKey, select, ctx); - const data = { __typename: typename }; let node: FieldNode | void; + let hasChanged = false; + const output = makeData(data); while ((node = iterate())) { const fieldAlias = getFieldAlias(node); - const fieldValue = originalData[fieldAlias]; + const fieldValue = output[fieldAlias]; // Add the current alias to the walked path before processing the field's value ctx.__internal.path.push(fieldAlias); - // Process the root field's value + // We temporarily store the data field in here, but undefined + // means that the value is missing from the cache + let dataFieldValue: void | DataField; if (node.selectionSet && fieldValue !== null) { - const fieldData = ensureData(fieldValue); - data[fieldAlias] = readRootField(ctx, getSelectionSet(node), fieldData); + dataFieldValue = readRootField( + ctx, + getSelectionSet(node), + ensureData(fieldValue) + ); } else { - data[fieldAlias] = fieldValue; + dataFieldValue = fieldValue; } + + // Check for any referential changes in the field's value + hasChanged = hasChanged || dataFieldValue !== output[fieldAlias]; + output[fieldAlias] = dataFieldValue!; + // After processing the field, remove the current alias from the path again ctx.__internal.path.pop(); } - return data; + return hasChanged ? output : data; }; const readRootField = ( @@ -156,16 +168,18 @@ const readRootField = ( ): Link => { if (Array.isArray(originalData)) { const newData = new Array(originalData.length); + let hasChanged = false; for (let i = 0, l = originalData.length; i < l; i++) { // Add the current index to the walked path before reading the field's value ctx.__internal.path.push(i); // Recursively read the root field's value newData[i] = readRootField(ctx, select, originalData[i]); + hasChanged = hasChanged || newData[i] !== originalData[i]; // After processing the field, remove the current index from the path ctx.__internal.path.pop(); } - return newData; + return hasChanged ? newData : originalData; } else if (originalData === null) { return null; } @@ -175,8 +189,7 @@ const readRootField = ( if (entityKey !== null) { // We assume that since this is used for result data this can never be undefined, // since the result data has already been written to the cache - const fieldValue = readSelection(ctx, entityKey, select, {} as Data); - return fieldValue === undefined ? null : fieldValue; + return readSelection(ctx, entityKey, select, originalData) || null; } else { return readRoot(ctx, originalData.__typename, select, originalData); } @@ -230,7 +243,7 @@ export const readFragment = ( ); const result = - readSelection(ctx, entityKey, getSelectionSet(fragment), {} as Data) || + readSelection(ctx, entityKey, getSelectionSet(fragment), makeData()) || null; if (process.env.NODE_ENV !== 'production') { @@ -287,9 +300,11 @@ const readSelection = ( const iterate = makeSelectionIterator(typename, entityKey, select, ctx); - let node: FieldNode | void; let hasFields = false; let hasPartials = false; + let hasChanged = typename !== data.__typename; + let node: FieldNode | void; + const output = makeData(data); while ((node = iterate()) !== undefined) { // Derive the needed data from our node. const fieldName = getName(node); @@ -305,19 +320,16 @@ const readSelection = ( isFieldAvailableOnType(store.schema, typename, fieldName); } - // We directly assign typenames and skip the field afterwards - if (fieldName === '__typename') { - data[fieldAlias] = typename; - continue; - } - + // Add the current alias to the walked path before processing the field's value + ctx.__internal.path.push(fieldAlias); // We temporarily store the data field in here, but undefined // means that the value is missing from the cache let dataFieldValue: void | DataField; - // Add the current alias to the walked path before processing the field's value - ctx.__internal.path.push(fieldAlias); - if (resultValue !== undefined && node.selectionSet === undefined) { + if (fieldName === '__typename') { + // We directly assign the typename as it's already available + dataFieldValue = typename; + } else if (resultValue !== undefined && node.selectionSet === undefined) { // The field is a scalar and can be retrieved directly from the result dataFieldValue = resultValue; } else if ( @@ -332,11 +344,11 @@ const readSelection = ( // We have a resolver for this field. // Prepare the actual fieldValue, so that the resolver can use it if (fieldValue !== undefined) { - data[fieldAlias] = fieldValue; + output[fieldAlias] = fieldValue; } dataFieldValue = resolvers[fieldName]( - data, + output, fieldArgs || ({} as Variables), store, ctx @@ -351,8 +363,9 @@ const readSelection = ( fieldName, key, getSelectionSet(node), - data[fieldAlias] as Data, - dataFieldValue + output[fieldAlias] as Data, + dataFieldValue, + ownsData(output) ); } @@ -376,8 +389,9 @@ const readSelection = ( fieldName, key, getSelectionSet(node), - data[fieldAlias] as Data, - resultValue + output[fieldAlias] as Data, + resultValue, + ownsData(output) ); } else { // Otherwise we attempt to get the missing field from the cache @@ -390,7 +404,8 @@ const readSelection = ( typename, fieldName, getSelectionSet(node), - data[fieldAlias] as Data + output[fieldAlias] as Data, + ownsData(output) ); } else if (typeof fieldValue === 'object' && fieldValue !== null) { // The entity on the field was invalid but can still be recovered @@ -398,39 +413,40 @@ const readSelection = ( } } - // If we have an error registered for the current field change undefined values to null - if (dataFieldValue === undefined && !!getFieldError(ctx)) { - hasPartials = true; - dataFieldValue = null; - } - - // After processing the field, remove the current alias from the path again - ctx.__internal.path.pop(); - // Now that dataFieldValue has been retrieved it'll be set on data // If it's uncached (undefined) but nullable we can continue assembling // a partial query result if ( dataFieldValue === undefined && - store.schema && - isFieldNullable(store.schema, typename, fieldName) + ((store.schema && isFieldNullable(store.schema, typename, fieldName)) || + !!getFieldError(ctx)) ) { - // The field is uncached but we have a schema that says it's nullable - // Set the field to null and continue + // The field is uncached or has errored, so it'll be set to null and skipped hasPartials = true; - data[fieldAlias] = null; - } else if (dataFieldValue === undefined) { - // The field is uncached and not nullable; return undefined - return undefined; + dataFieldValue = null; } else { // Otherwise continue as usual - hasFields = true; - data[fieldAlias] = dataFieldValue; + hasFields = fieldName !== '__typename'; + } + + // After processing the field, remove the current alias from the path again + ctx.__internal.path.pop(); + // Return undefined immediately when a field is uncached + if (dataFieldValue === undefined) { + return undefined; } + + // Check for any referential changes in the field's value + hasChanged = hasChanged || dataFieldValue !== output[fieldAlias]; + output[fieldAlias] = dataFieldValue; } - if (hasPartials) ctx.partial = true; - return isQuery && hasPartials && !hasFields ? undefined : data; + ctx.partial = ctx.partial || hasPartials; + return isQuery && hasPartials && !hasFields + ? undefined + : hasChanged + ? output + : data; }; const resolveResolverResult = ( @@ -440,7 +456,8 @@ const resolveResolverResult = ( key: string, select: SelectionSet, prevData: void | null | Data | Data[], - result: void | DataField + result: void | DataField, + skipNull: boolean ): DataField | void => { if (Array.isArray(result)) { const { store } = ctx; @@ -450,6 +467,8 @@ const resolveResolverResult = ( ? isListNullable(store.schema, typename, fieldName) : false; const data = new Array(result.length); + let hasChanged = + !Array.isArray(prevData) || result.length !== prevData.length; for (let i = 0, l = result.length; i < l; i++) { // Add the current index to the walked path before reading the field's value ctx.__internal.path.push(i); @@ -460,9 +479,9 @@ const resolveResolverResult = ( fieldName, joinKeys(key, `${i}`), select, - // Get the inner previous data from prevData prevData != null ? prevData[i] : undefined, - result[i] + result[i], + skipNull ); // After processing the field, remove the current index from the path ctx.__internal.path.pop(); @@ -471,18 +490,17 @@ const resolveResolverResult = ( return undefined; } else { data[i] = childResult !== undefined ? childResult : null; + hasChanged = hasChanged || data[i] !== prevData![i]; } } - return data; + return hasChanged ? data : prevData; } else if (result === null || result === undefined) { return result; - } else if (prevData === null) { - // If we've previously set this piece of data to be null, - // we skip it and return null immediately + } else if (skipNull && prevData === null) { return null; } else if (isDataOrKey(result)) { - const data = (prevData || {}) as Data; + const data = (prevData || makeData()) as Data; return typeof result === 'string' ? readSelection(ctx, result, select, data) : readSelection(ctx, key, select, data, result); @@ -505,7 +523,8 @@ const resolveLink = ( typename: string, fieldName: string, select: SelectionSet, - prevData: void | null | Data | Data[] + prevData: void | null | Data | Data[], + skipNull: boolean ): DataField | undefined => { if (Array.isArray(link)) { const { store } = ctx; @@ -513,6 +532,8 @@ const resolveLink = ( ? isListNullable(store.schema, typename, fieldName) : false; const newLink = new Array(link.length); + let hasChanged = + !Array.isArray(prevData) || newLink.length !== prevData.length; for (let i = 0, l = link.length; i < l; i++) { // Add the current index to the walked path before reading the field's value ctx.__internal.path.push(i); @@ -523,7 +544,8 @@ const resolveLink = ( typename, fieldName, select, - prevData != null ? prevData[i] : undefined + prevData != null ? prevData[i] : undefined, + skipNull ); // After processing the field, remove the current index from the path ctx.__internal.path.pop(); @@ -531,18 +553,17 @@ const resolveLink = ( if (childLink === undefined && !_isListNullable) { return undefined; } else { - newLink[i] = childLink !== undefined ? childLink : null; + newLink[i] = childLink || null; + hasChanged = hasChanged || newLink[i] !== prevData![i]; } } - return newLink; - } else if (link === null || prevData === null) { - // If the link is set to null or we previously set this piece of data to be null, - // we skip it and return null immediately + return hasChanged ? newLink : (prevData as Data[]); + } else if (link === null || (prevData === null && ownsData)) { return null; - } else { - return readSelection(ctx, link, select, (prevData || {}) as Data); } + + return readSelection(ctx, link, select, (prevData || makeData()) as Data); }; const isDataOrKey = (x: any): x is string | Data => diff --git a/exchanges/graphcache/src/operations/shared.ts b/exchanges/graphcache/src/operations/shared.ts index 2b883440fb..7931dc5864 100644 --- a/exchanges/graphcache/src/operations/shared.ts +++ b/exchanges/graphcache/src/operations/shared.ts @@ -212,7 +212,7 @@ export const makeSelectionIterator = ( }; export const ensureData = (x: DataField): Data | NullArray | null => - x === undefined ? null : (x as Data | NullArray); + x == null ? null : (x as Data | NullArray); export const ensureLink = (store: Store, ref: Link): Link => { if (ref == null) { diff --git a/exchanges/graphcache/src/store/data.ts b/exchanges/graphcache/src/store/data.ts index 797fc6ac05..b8270640e2 100644 --- a/exchanges/graphcache/src/store/data.ts +++ b/exchanges/graphcache/src/store/data.ts @@ -8,6 +8,7 @@ import { SerializedEntries, Dependencies, OperationType, + Data, } from '../types'; import { @@ -54,6 +55,8 @@ export interface InMemoryData { storage: StorageAdapter | null; } +let currentOwnership: null | Set = null; +let currentDataMapping: null | Map = null; let currentOperation: null | OperationType = null; let currentData: null | InMemoryData = null; let currentDependencies: null | Dependencies = null; @@ -65,6 +68,24 @@ const makeNodeMap = (): NodeMap => ({ base: new Map(), }); +/** Creates a new data object unless it's been created in this data run */ +export const makeData = (data?: Data): Data => { + let newData: Data; + if (data) { + if (currentOwnership!.has(data)) return data; + newData = currentDataMapping!.get(data) || ({ ...data } as Data); + currentDataMapping!.set(data, newData); + } else { + newData = {} as Data; + } + + currentOwnership!.add(newData); + return newData; +}; + +export const ownsData = (data?: Data): boolean => + !!data && currentOwnership!.has(data); + /** Before reading or writing the global state needs to be initialised */ export const initDataState = ( operationType: OperationType, @@ -72,6 +93,8 @@ export const initDataState = ( layerKey: number | null, isOptimistic?: boolean ) => { + currentOwnership = new Set(); + currentDataMapping = new Map(); currentOperation = operationType; currentData = data; currentDependencies = makeDict(); @@ -133,6 +156,8 @@ export const clearDataState = () => { } } + currentOwnership = null; + currentDataMapping = null; currentOperation = null; currentData = null; currentDependencies = null; diff --git a/exchanges/graphcache/src/store/index.ts b/exchanges/graphcache/src/store/index.ts index 1cfdb2ca37..b8b37fd55b 100644 --- a/exchanges/graphcache/src/store/index.ts +++ b/exchanges/graphcache/src/store/index.ts @@ -2,6 +2,8 @@ export { initDataState, clearDataState, noopDataState, + makeData, + ownsData, reserveLayer, getCurrentOperation, getCurrentDependencies,