From 126c9a97da94d6e66c75423500ed2d2ea31bfe65 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 20 Oct 2020 13:28:12 +0100 Subject: [PATCH 1/5] Add failing test case for cumulative optimistic updates --- .../src/test-utils/examples-1.test.ts | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) 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' }, + ], + }); +}); From f3ffd2da6837b6d523c8d51e88391aef9070d894 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 20 Oct 2020 13:28:25 +0100 Subject: [PATCH 2/5] Restrict `currentIgnoreOptimistic` condition to read --- exchanges/graphcache/src/store/data.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/exchanges/graphcache/src/store/data.ts b/exchanges/graphcache/src/store/data.ts index d1d31ab020..4d701a715e 100644 --- a/exchanges/graphcache/src/store/data.ts +++ b/exchanges/graphcache/src/store/data.ts @@ -75,7 +75,7 @@ export const initDataState = ( currentOperation = operationType; currentData = data; currentDependencies = makeDict(); - currentIgnoreOptimistic = !!isOptimistic; + currentIgnoreOptimistic = !!isOptimistic && operationType !== 'write'; if (process.env.NODE_ENV !== 'production') { currentDebugStack.length = 0; } From c58bbd5b5babafdff67e7b08de26cbef9a756267 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 20 Oct 2020 13:45:04 +0100 Subject: [PATCH 3/5] Fix condition to still skip persistence runs --- exchanges/graphcache/src/store/data.ts | 21 ++++++++++---------- exchanges/graphcache/src/store/store.test.ts | 2 +- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/exchanges/graphcache/src/store/data.ts b/exchanges/graphcache/src/store/data.ts index 4d701a715e..c80ce8202f 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 && operationType !== 'write'; + 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,7 @@ export const inspectFields = (entityKey: string): FieldInfo[] => { export const persistData = () => { if (currentData!.storage) { - currentIgnoreOptimistic = true; + currentOptimistic = true; const entries: SerializedEntries = makeDict(); currentData!.persist.forEach(key => { const { entityKey, fieldKey } = deserializeKeyInfo(key); @@ -571,7 +572,7 @@ export const persistData = () => { } }); - currentIgnoreOptimistic = false; + currentOptimistic = false; currentData!.storage.writeData(entries); currentData!.persist.clear(); } @@ -582,7 +583,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(); From 16c0f581cd24fcde6cbf15c3f8be8ccc6c41d08f Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 20 Oct 2020 13:47:46 +0100 Subject: [PATCH 4/5] Add changeset --- .changeset/ten-turkeys-appear.md | 5 +++++ 1 file changed, 5 insertions(+) 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. From db86066a4b12260998cbecba0b560d5ccacde5af Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 20 Oct 2020 14:05:53 +0100 Subject: [PATCH 5/5] Ensure that persistData isn't accidentally reading optimistic data This is just to prevent us from messing this up in the future. --- exchanges/graphcache/src/store/data.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/exchanges/graphcache/src/store/data.ts b/exchanges/graphcache/src/store/data.ts index c80ce8202f..aef31eba36 100644 --- a/exchanges/graphcache/src/store/data.ts +++ b/exchanges/graphcache/src/store/data.ts @@ -559,6 +559,7 @@ export const inspectFields = (entityKey: string): FieldInfo[] => { export const persistData = () => { if (currentData!.storage) { currentOptimistic = true; + currentOperation = 'read'; const entries: SerializedEntries = makeDict(); currentData!.persist.forEach(key => { const { entityKey, fieldKey } = deserializeKeyInfo(key);