From 28afd2320c0665feadc1bc557d2a0576d890db6d Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 20 Oct 2020 14:37:03 +0100 Subject: [PATCH] (graphcache) - Reenable cumulative optimistic updates (#1074) * Add failing test case for cumulative optimistic updates * Restrict `currentIgnoreOptimistic` condition to read * Fix condition to still skip persistence runs * Add changeset * Ensure that persistData isn't accidentally reading optimistic data This is just to prevent us from messing this up in the future. --- .changeset/ten-turkeys-appear.md | 5 ++ exchanges/graphcache/src/store/data.ts | 22 ++++--- exchanges/graphcache/src/store/store.test.ts | 2 +- .../src/test-utils/examples-1.test.ts | 63 +++++++++++++++++++ 4 files changed, 81 insertions(+), 11 deletions(-) create mode 100644 .changeset/ten-turkeys-appear.md diff --git a/.changeset/ten-turkeys-appear.md b/.changeset/ten-turkeys-appear.md new file mode 100644 index 0000000000..3ac9a9d84e --- /dev/null +++ b/.changeset/ten-turkeys-appear.md @@ -0,0 +1,5 @@ +--- +'@urql/exchange-graphcache': patch +--- + +Fix optimistic updates not being allowed to be cumulative and apply on top of each other. Previously in [#866](https://github.com/FormidableLabs/urql/pull/866) we explicitly deemed this as unsafe which isn't correct anymore given that concrete, non-optimistic updates are now never applied on top of optimistic layers. diff --git a/exchanges/graphcache/src/store/data.ts b/exchanges/graphcache/src/store/data.ts index d1d31ab020..aef31eba36 100644 --- a/exchanges/graphcache/src/store/data.ts +++ b/exchanges/graphcache/src/store/data.ts @@ -58,7 +58,7 @@ let currentOperation: null | OperationType = null; let currentData: null | InMemoryData = null; let currentDependencies: null | Dependencies = null; let currentOptimisticKey: null | number = null; -let currentIgnoreOptimistic = false; +let currentOptimistic = false; const makeNodeMap = (): NodeMap => ({ optimistic: makeDict(), @@ -75,7 +75,7 @@ export const initDataState = ( currentOperation = operationType; currentData = data; currentDependencies = makeDict(); - currentIgnoreOptimistic = !!isOptimistic; + currentOptimistic = !!isOptimistic; if (process.env.NODE_ENV !== 'production') { currentDebugStack.length = 0; } @@ -96,7 +96,7 @@ export const initDataState = ( // An optimistic update of a mutation may force an optimistic layer, // or this Query update may be applied optimistically since it's part - // of a commutate chain + // of a commutative chain currentOptimisticKey = layerKey; createLayer(data, layerKey); } else { @@ -116,7 +116,7 @@ export const clearDataState = () => { const data = currentData!; const layerKey = currentOptimisticKey; - currentIgnoreOptimistic = false; + currentOptimistic = false; currentOptimisticKey = null; // Determine whether the current operation has been a commutative layer @@ -144,7 +144,7 @@ export const clearDataState = () => { if (process.env.NODE_ENV !== 'test' && !data.defer) { data.defer = true; Promise.resolve().then(() => { - initDataState('write', data, null); + initDataState('read', data, null); gc(); persistData(); clearDataState(); @@ -246,7 +246,8 @@ const getNode = ( // If the node and node value exists it is returned, including undefined if ( optimistic && - (!currentIgnoreOptimistic || + (!currentOptimistic || + currentOperation === 'write' || currentData!.commutativeKeys.has(layerKey)) && (node = optimistic.get(entityKey)) !== undefined && fieldKey in node @@ -383,7 +384,7 @@ const updateDependencies = (entityKey: string, fieldKey?: string) => { }; const updatePersist = (entityKey: string, fieldKey: string) => { - if (!currentIgnoreOptimistic && currentData!.storage) { + if (!currentOptimistic && currentData!.storage) { currentData!.persist.add(serializeKeys(entityKey, fieldKey)); } }; @@ -557,7 +558,8 @@ export const inspectFields = (entityKey: string): FieldInfo[] => { export const persistData = () => { if (currentData!.storage) { - currentIgnoreOptimistic = true; + currentOptimistic = true; + currentOperation = 'read'; const entries: SerializedEntries = makeDict(); currentData!.persist.forEach(key => { const { entityKey, fieldKey } = deserializeKeyInfo(key); @@ -571,7 +573,7 @@ export const persistData = () => { } }); - currentIgnoreOptimistic = false; + currentOptimistic = false; currentData!.storage.writeData(entries); currentData!.persist.clear(); } @@ -582,7 +584,7 @@ export const hydrateData = ( storage: StorageAdapter, entries: SerializedEntries ) => { - initDataState('read', data, null); + initDataState('write', data, null); for (const key in entries) { const value = entries[key]; diff --git a/exchanges/graphcache/src/store/store.test.ts b/exchanges/graphcache/src/store/store.test.ts index e0a62ba592..60d00693cc 100644 --- a/exchanges/graphcache/src/store/store.test.ts +++ b/exchanges/graphcache/src/store/store.test.ts @@ -783,7 +783,7 @@ describe('Store with storage', () => { InMemoryData.writeRecord('Query', 'base', false); InMemoryData.clearDataState(); - InMemoryData.initDataState('write', store.data, null); + InMemoryData.initDataState('read', store.data, null); expect(InMemoryData.readRecord('Query', 'base')).toBe(false); InMemoryData.persistData(); InMemoryData.clearDataState(); diff --git a/exchanges/graphcache/src/test-utils/examples-1.test.ts b/exchanges/graphcache/src/test-utils/examples-1.test.ts index 23c4bf8dc0..b3a7031fed 100644 --- a/exchanges/graphcache/src/test-utils/examples-1.test.ts +++ b/exchanges/graphcache/src/test-utils/examples-1.test.ts @@ -489,3 +489,66 @@ it('correctly resolves optimistic updates on Relay schemas', () => { expect(queryRes.partial).toBe(false); expect(queryRes.data).not.toBe(null); }); + +it('allows cumulative optimistic updates', () => { + let counter = 1; + + const store = new Store({ + updates: { + Mutation: { + addTodo: (result, _, cache) => { + cache.updateQuery({ query: Todos }, data => { + (data as any).todos.push(result.addTodo); + return data as Data; + }); + }, + }, + }, + optimistic: { + addTodo: () => ({ + __typename: 'Todo', + id: 'optimistic_' + ++counter, + text: '', + complete: false, + }), + }, + }); + + const todosData = { + __typename: 'Query', + todos: [ + { id: '0', complete: true, text: '0', __typename: 'Todo' }, + { id: '1', complete: true, text: '1', __typename: 'Todo' }, + ], + }; + + write(store, { query: Todos }, todosData); + + const AddTodo = gql` + mutation { + __typename + addTodo { + __typename + complete + text + id + } + } + `; + + writeOptimistic(store, { query: AddTodo }, 1); + writeOptimistic(store, { query: AddTodo }, 2); + + const queryRes = query(store, { query: Todos }); + + expect(queryRes.partial).toBe(false); + expect(queryRes.data).toEqual({ + ...todosData, + todos: [ + todosData.todos[0], + todosData.todos[1], + { __typename: 'Todo', text: '', complete: false, id: 'optimistic_2' }, + { __typename: 'Todo', text: '', complete: false, id: 'optimistic_3' }, + ], + }); +});