Repository: FormidableLabs/urql
Pull request #588 (2020-03-08 23:15:00)

Fix teardown operation breaking commutative layers
Previously a commutative layer would be deleted when a `teardown` event came in, which wasn't respecting layers that may already have been written to. Instead the `teardown` now triggers an empty data change, which will only delete the layer if it exists and if it can be.

changeset-bot[bot]: ###  🦋  Changeset is good to go

Latest commit: 25145862e03a5601aa560d7625aa49176716df1d

**We got this.**

Not sure what this means? [Click here  to learn what changesets are](https://github.com/changesets/changesets/blob/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add another changeset to this PR](https://github.com/FormidableLabs/urql/new/fix/teardown-for-commutativity?filename=.changeset/strange-donkeys-learn.md&value=---%0A%22%40fake-scope%2Ffake-pkg%22%3A%20patch%0A---%0A%0AUpdate%20the%20package%20info%20above%20to%20the%20changes%20you%20want%2C%20and%20write%20your%20description%20here.%20You%20can%20add%20more%20packages%20on%20new%20lines.%0A)



Patch:
diff --git a/.changeset/two-guests-hide.md b/.changeset/two-guests-hide.md
new file mode 100644
index 00000000..d09a482d
--- /dev/null
+++ b/.changeset/two-guests-hide.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Fix teardown events leading to broken commutativity
diff --git a/exchanges/graphcache/src/cacheExchange.ts b/exchanges/graphcache/src/cacheExchange.ts
index 9c1fe7b4..7ffe86c3 100644
--- a/exchanges/graphcache/src/cacheExchange.ts
+++ b/exchanges/graphcache/src/cacheExchange.ts
@@ -29,7 +29,7 @@ import {
 import { query, write, writeOptimistic } from './operations';
 import { hydrateData } from './store/data';
 import { makeDict } from './helpers/dict';
-import { Store, clearLayer, reserveLayer } from './store';
+import { Store, noopDataState, clearLayer, reserveLayer } from './store';
 
 import {
   UpdatesConfig,
@@ -174,7 +174,7 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({
     if (operation.operationName === 'query') {
       reserveLayer(store.data, operation.key);
     } else if (operation.operationName === 'teardown') {
-      clearLayer(store.data, operation.key);
+      noopDataState(store.data, operation.key);
     }
   };
 
diff --git a/exchanges/graphcache/src/store/data.ts b/exchanges/graphcache/src/store/data.ts
index 44b7ad53..32104f8d 100644
--- a/exchanges/graphcache/src/store/data.ts
+++ b/exchanges/graphcache/src/store/data.ts
@@ -135,6 +135,12 @@ export const clearDataState = () => {
   }
 };
 
+/** Initialises then resets the data state, which may squash this layer if necessary */
+export const noopDataState = (data: InMemoryData, layerKey: number | null) => {
+  initDataState(data, layerKey);
+  clearDataState();
+};
+
 /** As we're writing, we keep around all the records and links we've read or have written to */
 export const getCurrentDependencies = (): Set<string> => {
   invariant(
diff --git a/exchanges/graphcache/src/store/index.ts b/exchanges/graphcache/src/store/index.ts
index 6fc20d84..f7b4b6f5 100644
--- a/exchanges/graphcache/src/store/index.ts
+++ b/exchanges/graphcache/src/store/index.ts
@@ -1,6 +1,7 @@
 export {
   initDataState,
   clearDataState,
+  noopDataState,
   reserveLayer,
   clearLayer,
   getCurrentDependencies,


Test patch:
diff --git a/exchanges/graphcache/src/cacheExchange.test.ts b/exchanges/graphcache/src/cacheExchange.test.ts
index a78e9301..90471d57 100644
--- a/exchanges/graphcache/src/cacheExchange.test.ts
+++ b/exchanges/graphcache/src/cacheExchange.test.ts
@@ -10,6 +10,7 @@ import {
   pipe,
   map,
   mergeMap,
+  filter,
   fromValue,
   makeSubject,
   tap,
@@ -1043,16 +1044,21 @@ it('applies results that come in out-of-order commutatively and consistently', (
     data = result.data;
   });
 
-  pipe(
-    cacheExchange()({ forward: ops$ => pipe(ops$, mergeMap(result)), client })(
-      ops$
-    ),
-    tap(output),
-    publish
-  );
+  const forward = (ops$: Source<Operation>): Source<OperationResult> =>
+    pipe(
+      ops$,
+      filter(op => op.operationName !== 'teardown'),
+      mergeMap(result)
+    );
+
+  pipe(cacheExchange()({ forward, client })(ops$), tap(output), publish);
 
   next(client.createRequestOperation('query', { key: 1, query }));
   next(client.createRequestOperation('query', { key: 2, query }));
+
+  // This shouldn't have any effect:
+  next(client.createRequestOperation('teardown', { key: 2, query }));
+
   next(client.createRequestOperation('query', { key: 3, query }));
 
   jest.advanceTimersByTime(5);
diff --git a/exchanges/graphcache/src/store/data.test.ts b/exchanges/graphcache/src/store/data.test.ts
index d5b9b157..394e01c5 100644
--- a/exchanges/graphcache/src/store/data.test.ts
+++ b/exchanges/graphcache/src/store/data.test.ts
@@ -323,4 +323,15 @@ describe('commutative changes', () => {
 
     expect(data.optimisticOrder).toEqual([]);
   });
+
+  it('allows noopDataState to clear layers only if necessary', () => {
+    InMemoryData.reserveLayer(data, 1);
+    InMemoryData.reserveLayer(data, 2);
+
+    InMemoryData.noopDataState(data, 2);
+    expect(data.optimisticOrder).toEqual([2, 1]);
+
+    InMemoryData.noopDataState(data, 1);
+    expect(data.optimisticOrder).toEqual([]);
+  });
 });