diff --git a/.changeset/smart-lizards-crash.md b/.changeset/smart-lizards-crash.md new file mode 100644 index 0000000000..cdff4b5e80 --- /dev/null +++ b/.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"`. diff --git a/docs/graphcache/errors.md b/docs/graphcache/errors.md index 86dc97c5ad..6d156d0af5 100644 --- a/docs/graphcache/errors.md +++ b/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, diff --git a/exchanges/graphcache/default-storage/package.json b/exchanges/graphcache/default-storage/package.json index 1d3a886e45..645fc708d6 100644 --- a/exchanges/graphcache/default-storage/package.json +++ b/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" } } diff --git a/exchanges/graphcache/src/cacheExchange.test.ts b/exchanges/graphcache/src/cacheExchange.test.ts index 072f8a59c8..605dcfc7e5 100644 --- a/exchanges/graphcache/src/cacheExchange.test.ts +++ b/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', diff --git a/exchanges/graphcache/src/extras/relayPagination.test.ts b/exchanges/graphcache/src/extras/relayPagination.test.ts index fe75ad0aab..7b74d79a0b 100644 --- a/exchanges/graphcache/src/extras/relayPagination.test.ts +++ b/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 { diff --git a/exchanges/graphcache/src/extras/simplePagination.test.ts b/exchanges/graphcache/src/extras/simplePagination.test.ts index fd2e60b84d..51ffdf7016 100644 --- a/exchanges/graphcache/src/extras/simplePagination.test.ts +++ b/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 diff --git a/exchanges/graphcache/src/helpers/help.ts b/exchanges/graphcache/src/helpers/help.ts index 86d54fc422..7454272b04 100644 --- a/exchanges/graphcache/src/helpers/help.ts +++ b/exchanges/graphcache/src/helpers/help.ts @@ -19,6 +19,7 @@ export type ErrorCode = | 11 | 12 | 13 + | 14 | 15 | 16 | 17 diff --git a/exchanges/graphcache/src/offlineExchange.test.ts b/exchanges/graphcache/src/offlineExchange.test.ts index 048e23a0e9..5c5dea5401 100644 --- a/exchanges/graphcache/src/offlineExchange.test.ts +++ b/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' }], }); }); diff --git a/exchanges/graphcache/src/operations/query.test.ts b/exchanges/graphcache/src/operations/query.test.ts index 4eb14dfa76..be81ebc315 100644 --- a/exchanges/graphcache/src/operations/query.test.ts +++ b/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 } } diff --git a/exchanges/graphcache/src/operations/query.ts b/exchanges/graphcache/src/operations/query.ts index ecd1a3f349..d40ed7cc2e 100644 --- a/exchanges/graphcache/src/operations/query.ts +++ b/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 ( diff --git a/exchanges/graphcache/src/operations/shared.ts b/exchanges/graphcache/src/operations/shared.ts index fe43224e9b..787e5c86b7 100644 --- a/exchanges/graphcache/src/operations/shared.ts +++ b/exchanges/graphcache/src/operations/shared.ts @@ -149,7 +149,7 @@ export const makeSelectionIterator = ( ))(); } } - } else if (getName(node) !== '__typename') { + } else { return node; } } diff --git a/exchanges/graphcache/src/operations/write.test.ts b/exchanges/graphcache/src/operations/write.test.ts index 44e65ce64f..b6e4a4d4f1 100644 --- a/exchanges/graphcache/src/operations/write.test.ts +++ b/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`/ ); diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts index a4872a1573..3a3a42d479 100644 --- a/exchanges/graphcache/src/operations/write.ts +++ b/exchanges/graphcache/src/operations/write.ts @@ -194,6 +194,11 @@ const writeSelection = ( const isRoot = !isQuery && !!ctx.store.rootNames[entityKey!]; const typename = isRoot || isQuery ? entityKey : data.__typename; if (!typename) { + warn( + "Couldn't find __typename when writing.\n" + + "If you're writing to the cache manually have to pass a `__typename` property on each entity in your data.", + 14 + ); return; } else if (!isRoot && !isQuery && entityKey) { InMemoryData.writeRecord(entityKey, '__typename', typename); @@ -236,12 +241,14 @@ const writeSelection = ( ); continue; // Skip this field - } else if (ctx.store.schema && typename) { + } else if (ctx.store.schema && typename && fieldName !== '__typename') { isFieldAvailableOnType(ctx.store.schema, typename, fieldName); } } - if (ctx.optimistic && isRoot) { + if (fieldName === '__typename') { + continue; + } else if (ctx.optimistic && isRoot) { const resolver = ctx.store.optimisticMutations[fieldName]; if (!resolver) continue; diff --git a/exchanges/graphcache/src/store/store.test.ts b/exchanges/graphcache/src/store/store.test.ts index 60d00693cc..43c816489f 100644 --- a/exchanges/graphcache/src/store/store.test.ts +++ b/exchanges/graphcache/src/store/store.test.ts @@ -2,6 +2,7 @@ import gql from 'graphql-tag'; import { minifyIntrospectionQuery } from '@urql/introspection'; +import { maskTypename } from '@urql/core'; import { mocked } from 'ts-jest/utils'; import { Data, StorageAdapter } from '../types'; @@ -14,6 +15,7 @@ import { getIntrospectionQuery, parse } from 'graphql'; const Appointment = gql` query appointment($id: String) { + __typename appointment(id: $id) { __typename id @@ -41,6 +43,7 @@ const Todos = gql` const TodosWithoutTypename = gql` query { + __typename todos { id text @@ -87,7 +90,10 @@ describe('Store', () => { // NOTE: This is the query without __typename annotations write(store, { query: TodosWithoutTypename }, todosData); const result = query(store, { query: TodosWithoutTypename }); - expect(result.data).toEqual(todosData); + expect(result.data).toEqual({ + ...maskTypename(todosData), + __typename: 'Query', + }); }); }); @@ -473,6 +479,7 @@ describe('Store with OptimisticMutationConfig', () => { id text complete + __typename } `, { id: '0' } @@ -714,6 +721,7 @@ describe('Store with storage', () => { it('should be able to persist embedded data', () => { const EmbeddedAppointment = gql` query appointment($id: String) { + __typename appointment(id: $id) { __typename info @@ -803,7 +811,7 @@ describe('Store with storage', () => { InMemoryData.clearDataState(); }); - it("should warn if an optimistic field doesn't exist in the schema's mutations", function () { + it("should warn if an optimistic field doesn't exist in the schema's mutations", () => { new Store({ schema: minifyIntrospectionQuery( require('../test-utils/simple_schema.json') @@ -876,4 +884,35 @@ describe('Store with storage', () => { query(store, { query: parse(getIntrospectionQuery()) }, schema); expect(console.warn).toBeCalledTimes(0); }); + + it('should warn when __typename is missing when store.writeFragment is called', () => { + InMemoryData.initDataState('write', store.data, null); + + store.writeFragment( + parse(` + fragment _ on Test { + __typename + id + sub { + id + } + } + `), + { + id: 'test', + sub: { + id: 'test', + }, + } + ); + + InMemoryData.clearDataState(); + + expect(console.warn).toBeCalledTimes(1); + const warnMessage = mocked(console.warn).mock.calls[0][0]; + expect(warnMessage).toContain( + "Couldn't find __typename when writing.\nIf you're writing to the cache manually have to pass a `__typename` property on each entity in your data." + ); + expect(warnMessage).toContain('https://bit.ly/2XbVrpR#14'); + }); }); diff --git a/exchanges/graphcache/src/store/store.ts b/exchanges/graphcache/src/store/store.ts index d9ecd40532..65d7bccf0e 100644 --- a/exchanges/graphcache/src/store/store.ts +++ b/exchanges/graphcache/src/store/store.ts @@ -5,7 +5,7 @@ import { GraphQLSchema, } from 'graphql'; -import { TypedDocumentNode, createRequest } from '@urql/core'; +import { TypedDocumentNode, formatDocument, createRequest } from '@urql/core'; import { Cache, @@ -177,6 +177,7 @@ export class Store implements Cache { updater: (data: T | null) => T | null ): void { const request = createRequest(input.query, input.variables as any); + request.query = formatDocument(request.query); const output = updater(this.readQuery(request)); if (output !== null) { startWrite(this, request, output as any); @@ -184,8 +185,9 @@ export class Store implements Cache { } readQuery(input: QueryInput): T | null { - return read(this, createRequest(input.query, input.variables!)) - .data as T | null; + const request = createRequest(input.query, input.variables!); + request.query = formatDocument(request.query); + return read(this, request).data as T | null; } readFragment( @@ -193,7 +195,12 @@ export class Store implements Cache { entity: string | Data | T, variables?: V ): T | null { - return readFragment(this, fragment, entity, variables as any) as T | null; + return readFragment( + this, + formatDocument(fragment), + entity, + variables as any + ) as T | null; } writeFragment( @@ -201,6 +208,6 @@ export class Store implements Cache { data: T, variables?: V ): void { - writeFragment(this, fragment, data, variables as any); + writeFragment(this, formatDocument(fragment), data, variables as any); } } diff --git a/exchanges/graphcache/src/test-utils/examples-1.test.ts b/exchanges/graphcache/src/test-utils/examples-1.test.ts index 0dff0159fb..4378c408a3 100644 --- a/exchanges/graphcache/src/test-utils/examples-1.test.ts +++ b/exchanges/graphcache/src/test-utils/examples-1.test.ts @@ -63,6 +63,10 @@ const NestedClearNameTodo = gql` } `; +afterEach(() => { + expect(console.warn).not.toHaveBeenCalled(); +}); + it('passes the "getting-started" example', () => { const store = new Store(); const todosData = { @@ -146,6 +150,7 @@ it('resolves missing, nullable arguments on fields', () => { const GetWithVariables = gql` query { + __typename todo(first: null) { __typename id @@ -155,6 +160,7 @@ it('resolves missing, nullable arguments on fields', () => { const GetWithoutVariables = gql` query { + __typename todo { __typename id @@ -203,7 +209,6 @@ it('should link entities', () => { id: '0', text: 'Go to the shops', complete: false, - __typename: 'Todo', }, }); }); diff --git a/exchanges/graphcache/src/test-utils/examples-2.test.ts b/exchanges/graphcache/src/test-utils/examples-2.test.ts index 4eb674f246..982f3f6ccb 100644 --- a/exchanges/graphcache/src/test-utils/examples-2.test.ts +++ b/exchanges/graphcache/src/test-utils/examples-2.test.ts @@ -51,6 +51,10 @@ const Pagination = gql` } `; +afterEach(() => { + expect(console.warn).not.toHaveBeenCalled(); +}); + it('allows custom resolvers to resolve nested, unkeyed data', () => { const store = new Store({ resolvers: { @@ -98,7 +102,6 @@ it('allows custom resolvers to resolve nested, unkeyed data', () => { expect(res.partial).toBe(false); expect(res.data).toEqual({ - __typename: 'Query', todos: { __typename: 'TodosConnection', edges: [ @@ -161,7 +164,6 @@ it('allows custom resolvers to resolve nested, unkeyed data with embedded links' const res = query(store, { query: Pagination }); expect(res.partial).toBe(false); expect(res.data).toEqual({ - __typename: 'Query', todos: { __typename: 'TodosConnection', edges: [ @@ -186,6 +188,9 @@ it('allows custom resolvers to resolve nested, unkeyed data with embedded links' it('allows custom resolvers to resolve mixed data (keyable and unkeyable)', () => { const store = new Store({ + keys: { + TodoDetails: () => null, + }, resolvers: { Query: { todo: () => ({ @@ -224,7 +229,6 @@ it('allows custom resolvers to resolve mixed data (keyable and unkeyable)', () = expect(res.partial).toBe(false); expect(res.dependencies).toHaveProperty('Author:x', true); expect(res.data).toEqual({ - __typename: 'Query', todo: { __typename: 'Todo', id: '1', diff --git a/exchanges/graphcache/src/test-utils/examples-3.test.ts b/exchanges/graphcache/src/test-utils/examples-3.test.ts index 906f02ca1f..7ed830205f 100644 --- a/exchanges/graphcache/src/test-utils/examples-3.test.ts +++ b/exchanges/graphcache/src/test-utils/examples-3.test.ts @@ -2,6 +2,10 @@ import gql from 'graphql-tag'; import { query, write } from '../operations'; import { Store } from '../store'; +afterEach(() => { + expect(console.warn).not.toHaveBeenCalled(); +}); + it('allows viewer fields to overwrite the root Query data', () => { const store = new Store(); const get = gql` @@ -47,7 +51,6 @@ it('allows viewer fields to overwrite the root Query data', () => { expect(res.partial).toBe(false); expect(res.data).toEqual({ - __typename: 'Query', int: 43, }); }); diff --git a/exchanges/graphcache/src/test-utils/suite.test.ts b/exchanges/graphcache/src/test-utils/suite.test.ts index 0a8fe5a505..1f8958bc96 100644 --- a/exchanges/graphcache/src/test-utils/suite.test.ts +++ b/exchanges/graphcache/src/test-utils/suite.test.ts @@ -256,6 +256,7 @@ it('nested entity list on query', () => { expectCacheIntegrity({ query: gql` { + __typename items { __typename id @@ -377,6 +378,7 @@ it('embedded objects on entities', () => { expectCacheIntegrity({ query: gql` { + __typename user { __typename id