(graphcache) - Increase consistency of __typename in cached results (#…

…1185)

* Add `__typename` field as instructed by selection sets

The `__typename` field does not need to be added to root
types by default and must not be added first, as it shouldn't
be necessary. Instead we can assume that it's in the result
and copy it over once it's reached in the selection iteration.

* Update testing fixtures for query.test.ts

* Add changeset

* Update various test suites

* Restore skipped __typename field on write

* Update __typename field assignment to add alias

* Format documents before reading from cache in Store

* Add warning for writing selection sets without __typename

* Check test suites for silent warnings

* Update changeset

* Update Changeset to minor bump

I believe this should be a minor bump since we're adding a new
warning.

.changeset/smart-lizards-crash.md

@@ -0,0 +1,9 @@
+---
+'@urql/exchange-graphcache': minor
+---
+
+Increase the consistency of when and how the `__typename` field is added to results. Instead of
+adding it by default and automatically first, the `__typename` field will now be added along with
+the usual selection set. The `write` operation now automatically issues a warning if `__typename`
+isn't present where it's expected more often, which helps in debugging. Also the `__typename` field
+may now not proactively be added to root results, e.g. `"Query"`.

docs/graphcache/errors.md

@@ -194,6 +194,17 @@ As data is written to the cache, this warning is issued when `undefined` is enco
 GraphQL results should never contain an `undefined` value, so this warning will let you
 know which part of your result did contain `undefined`.
 
+## (14) Couldn't find \_\_typename when writing.
+
+> Couldn't find **typename when writing.
+> If you're writing to the cache manually have to pass a `**typename` property on each entity in your data.
+
+You probably have called `cache.writeFragment` or `cache.updateQuery` with data that is missing a
+`__typename` field for an entity where your document contains a selection set. The cache won't be
+able to generate a key for entities that are missing the `__typename` field.
+
+Please make sure that you include enough properties on your data so that `write` can generate a key.
+
 ## (15) Invalid key
 
 > Invalid key: The GraphQL query at the field at `???` has a selection set,

exchanges/graphcache/default-storage/package.json

@@ -15,7 +15,7 @@
     "./package.json": "./package.json"
   },
   "dependencies": {
-    "@urql/core": ">=1.14.1",
+    "@urql/core": ">=1.15.2",
     "wonka": "^4.0.14"
   }
 }

exchanges/graphcache/src/cacheExchange.test.ts

@@ -1011,6 +1011,7 @@ describe('optimistic updates', () => {
     expect(reexec).toHaveBeenCalledTimes(1);
 
     jest.runAllTimers();
+
     expect(updates.Mutation.addAuthor).toHaveBeenCalledTimes(2);
     expect(response).toHaveBeenCalledTimes(2);
     expect(result).toHaveBeenCalledTimes(4);
@@ -1456,7 +1457,6 @@ describe('schema awareness', () => {
     expect(reexec).toHaveBeenCalledTimes(1);
     expect(result.mock.calls[1][0].stale).toBe(true);
     expect(result.mock.calls[1][0].data).toEqual({
-      __typename: 'Query',
       todos: [
         {
           __typename: 'Todo',

exchanges/graphcache/src/extras/relayPagination.test.ts

@@ -20,6 +20,7 @@ function itemEdge(numItem: number) {
 it('works with forward pagination', () => {
   const Pagination = gql`
     query($cursor: String) {
+      __typename
       items(first: 1, after: $cursor) {
         __typename
         edges {
@@ -97,6 +98,7 @@ it('works with forward pagination', () => {
 it('works with backwards pagination', () => {
   const Pagination = gql`
     query($cursor: String) {
+      __typename
       items(last: 1, before: $cursor) {
         __typename
         edges {
@@ -174,6 +176,7 @@ it('works with backwards pagination', () => {
 it('handles duplicate edges', () => {
   const Pagination = gql`
     query($cursor: String) {
+      __typename
       items(first: 2, after: $cursor) {
         __typename
         edges {
@@ -259,6 +262,7 @@ it('handles duplicate edges', () => {
 it('works with simultaneous forward and backward pagination (outwards merging)', () => {
   const Pagination = gql`
     query($first: Int, $last: Int, $before: String, $after: String) {
+      __typename
       items(first: $first, last: $last, before: $before, after: $after) {
         __typename
         edges {
@@ -389,6 +393,7 @@ it('works with simultaneous forward and backward pagination (outwards merging)',
 it('works with simultaneous forward and backward pagination (inwards merging)', () => {
   const Pagination = gql`
     query($first: Int, $last: Int, $before: String, $after: String) {
+      __typename
       items(first: $first, last: $last, before: $before, after: $after) {
         __typename
         edges {
@@ -652,7 +657,9 @@ it('returns an empty array of edges when the cache has zero edges stored', () =>
 it('returns other fields on the same level as the edges', () => {
   const Pagination = gql`
     query {
+      __typename
       items(first: 1) {
+        __typename
         totalCount
       }
     }
@@ -691,6 +698,7 @@ it('returns other fields on the same level as the edges', () => {
 it('returns a subset of the cached items if the query requests less items than the cached ones', () => {
   const Pagination = gql`
     query($first: Int, $last: Int, $before: String, $after: String) {
+      __typename
       items(first: $first, last: $last, before: $before, after: $after) {
         __typename
         edges {
@@ -754,6 +762,7 @@ it('returns a subset of the cached items if the query requests less items than t
 it("returns the cached items even if they don't fullfil the query", () => {
   const Pagination = gql`
     query($first: Int, $last: Int, $before: String, $after: String) {
+      __typename
       items(first: $first, last: $last, before: $before, after: $after) {
         __typename
         edges {
@@ -821,6 +830,7 @@ it("returns the cached items even if they don't fullfil the query", () => {
 it('returns the cached items even when they come from a different query', () => {
   const Pagination = gql`
     query($first: Int, $last: Int, $before: String, $after: String) {
+      __typename
       items(first: $first, last: $last, before: $before, after: $after) {
         __typename
         edges {
@@ -884,6 +894,7 @@ it('returns the cached items even when they come from a different query', () =>
 it('caches and retrieves correctly queries with inwards pagination', () => {
   const Pagination = gql`
     query($first: Int, $last: Int, $before: String, $after: String) {
+      __typename
       items(first: $first, last: $last, before: $before, after: $after) {
         __typename
         edges {
@@ -951,6 +962,7 @@ it('caches and retrieves correctly queries with inwards pagination', () => {
 it('does not include a previous result when adding parameters', () => {
   const Pagination = gql`
     query($first: Int, $filter: String) {
+      __typename
       items(first: $first, filter: $filter) {
         __typename
         edges {
@@ -1033,6 +1045,7 @@ it('does not include a previous result when adding parameters', () => {
 it('Works with edges absent from query', () => {
   const Pagination = gql`
     query($first: Int, $last: Int, $before: String, $after: String) {
+      __typename
       items(first: $first, last: $last, before: $before, after: $after) {
         __typename
         nodes {
@@ -1088,6 +1101,7 @@ it('Works with edges absent from query', () => {
 it('Works with nodes absent from query', () => {
   const Pagination = gql`
     query($first: Int, $last: Int, $before: String, $after: String) {
+      __typename
       items(first: $first, last: $last, before: $before, after: $after) {
         __typename
         edges {

exchanges/graphcache/src/extras/simplePagination.test.ts

@@ -6,6 +6,7 @@ import { simplePagination } from './simplePagination';
 it('works with forward pagination', () => {
   const Pagination = gql`
     query($skip: Number, $limit: Number) {
+      __typename
       persons(skip: $skip, limit: $limit) {
         __typename
         id
@@ -76,6 +77,7 @@ it('works with forward pagination', () => {
 it('works with backwards pagination', () => {
   const Pagination = gql`
     query($skip: Number, $limit: Number) {
+      __typename
       persons(skip: $skip, limit: $limit) {
         __typename
         id
@@ -146,6 +148,7 @@ it('works with backwards pagination', () => {
 it('handles duplicates', () => {
   const Pagination = gql`
     query($skip: Number, $limit: Number) {
+      __typename
       persons(skip: $skip, limit: $limit) {
         __typename
         id
@@ -204,6 +207,7 @@ it('handles duplicates', () => {
 it('should not return previous result when adding a parameter', () => {
   const Pagination = gql`
     query($skip: Number, $limit: Number, $filter: String) {
+      __typename
       persons(skip: $skip, limit: $limit, filter: $filter) {
         __typename
         id
@@ -255,6 +259,7 @@ it('should not return previous result when adding a parameter', () => {
 it('should preserve the correct order in forward pagination', () => {
   const Pagination = gql`
     query($skip: Number, $limit: Number) {
+      __typename
       persons(skip: $skip, limit: $limit) {
         __typename
         id
@@ -313,6 +318,7 @@ it('should preserve the correct order in forward pagination', () => {
 it('should preserve the correct order in backward pagination', () => {
   const Pagination = gql`
     query($skip: Number, $limit: Number) {
+      __typename
       persons(skip: $skip, limit: $limit) {
         __typename
         id
@@ -362,6 +368,7 @@ it('should preserve the correct order in backward pagination', () => {
     query: Pagination,
     variables: { skip: 3, limit: 3 },
   });
+
   expect(result.data).toEqual({
     __typename: 'Query',
     persons: [...pageTwo.persons, ...pageOne.persons],
@@ -371,6 +378,7 @@ it('should preserve the correct order in backward pagination', () => {
 it('prevents overlapping of pagination on different arguments', () => {
   const Pagination = gql`
     query($skip: Number, $limit: Number, $filter: string) {
+      __typename
       persons(skip: $skip, limit: $limit, filter: $filter) {
         __typename
         id

exchanges/graphcache/src/helpers/help.ts

@@ -19,6 +19,7 @@ export type ErrorCode =
   | 11
   | 12
   | 13
+  | 14
   | 15
   | 16
   | 17

exchanges/graphcache/src/offlineExchange.test.ts

@@ -33,6 +33,7 @@ const queryOne = gql`
     authors {
       id
       name
+      __typename
     }
   }
 `;
@@ -41,9 +42,9 @@ const queryOneData = {
   __typename: 'Query',
   authors: [
     {
-      __typename: 'Author',
       id: '123',
       name: 'Me',
+      __typename: 'Author',
     },
   ],
 };
@@ -189,7 +190,6 @@ describe('offline', () => {
     next(queryOp);
     expect(result).toBeCalledTimes(2);
     expect(result.mock.calls[1][0].data).toEqual({
-      __typename: 'Query',
       authors: [{ id: '123', name: 'URQL', __typename: 'Author' }],
     });
   });

exchanges/graphcache/src/operations/query.test.ts

@@ -55,7 +55,6 @@ describe('Query', () => {
     const result = query(store, { query: TODO_QUERY });
     expect(result.partial).toBe(true);
     expect(result.data).toEqual({
-      __typename: 'Query',
       todos: [
         {
           id: '0',
@@ -135,7 +134,9 @@ describe('Query', () => {
   it('should not crash for valid queries', () => {
     const VALID_QUERY = gql`
       query getTodos {
+        __typename
         todos {
+          __typename
           id
           text
         }
@@ -162,14 +163,17 @@ describe('Query', () => {
       todos: [{ __typename: 'Todo', id: '0', text: 'Solve bug' }],
     });
 
-    expect(console.warn).not.toHaveBeenCalled();
+    // The warning should be called for `__typename`
+    expect(console.warn).toHaveBeenCalledTimes(1);
     expect(console.error).not.toHaveBeenCalled();
   });
 
   it('should respect altered root types', () => {
     const QUERY = gql`
       query getTodos {
+        __typename
         todos {
+          __typename
           id
           text
         }
@@ -203,7 +207,9 @@ describe('Query', () => {
   it('should allow subsequent read when first result was null', () => {
     const QUERY_WRITE = gql`
       query writeTodos {
+        __typename
         todos {
+          __typename
           ...ValidRead
         }
       }
@@ -215,10 +221,13 @@ describe('Query', () => {
 
     const QUERY_READ = gql`
       query getTodos {
+        __typename
         todos {
+          __typename
           ...MissingRead
         }
         todos {
+          __typename
           id
         }
       }

exchanges/graphcache/src/operations/query.ts

@@ -271,9 +271,6 @@ const readSelection = (
     return;
   }
 
-  // The following closely mirrors readSelection, but differs only slightly for the
-  // sake of resolving from an existing resolver result
-  data.__typename = typename;
   const iterate = makeSelectionIterator(typename, entityKey, select, ctx);
 
   let node: FieldNode | void;
@@ -298,7 +295,10 @@ const readSelection = (
     // means that the value is missing from the cache
     let dataFieldValue: void | DataField;
 
-    if (resultValue !== undefined && node.selectionSet === undefined) {
+    if (fieldName === '__typename') {
+      data[fieldAlias] = typename;
+      continue;
+    } else if (resultValue !== undefined && node.selectionSet === undefined) {
       // The field is a scalar and can be retrieved directly from the result
       dataFieldValue = resultValue;
     } else if (

exchanges/graphcache/src/operations/shared.ts

@@ -149,7 +149,7 @@ export const makeSelectionIterator = (
             ))();
           }
         }
-      } else if (getName(node) !== '__typename') {
+      } else {
         return node;
       }
     }

exchanges/graphcache/src/operations/write.test.ts

@@ -148,24 +148,7 @@ describe('Query', () => {
       }
     );
     // Because of us indicating Todo:Writer as a scalar
-    expect(console.warn).toHaveBeenCalledTimes(1);
-    write(
-      store,
-      { query: INVALID_TODO_QUERY },
-      {
-        __typename: 'Mutation',
-        toggleTodo: {
-          __typename: 'Todo',
-          id: '0',
-          text: 'Teach',
-          writer: {
-            id: '0',
-          },
-        },
-      }
-    );
-
-    expect(console.warn).toHaveBeenCalledTimes(1);
+    expect(console.warn).toHaveBeenCalledTimes(2);
     expect((console.warn as any).mock.calls[0][0]).toMatch(
       /The field `writer` does not exist on `Todo`/
     );