diff --git a/CHANGELOG.md b/CHANGELOG.md index d1148691e61..37faf75c9ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +## Apollo Client 3.4.10 (not yet released) + +### Improvements + +- Warn when calling `refetch({ variables })` instead of `refetch(variables)`, except for queries that declare a variable named `$variables` (uncommon).
+ [@benjamn](https://github.com/benjamn) in [#8702](https://github.com/apollographql/apollo-client/pull/8702) + ## Apollo Client 3.4.9 ### Bug Fixes diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index 6038a87755f..9bd4808a5fb 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -13,6 +13,7 @@ import { iterateObserversSafely, isNonEmptyArray, fixObservableSubclass, + getQueryDefinition, } from '../utilities'; import { ApolloError } from '../errors'; import { QueryManager } from './QueryManager'; @@ -26,6 +27,11 @@ import { import { QueryInfo } from './QueryInfo'; import { MissingFieldError } from '../cache'; +const { + assign, + hasOwnProperty, +} = Object; + export interface FetchMoreOptions< TData = any, TVariables = OperationVariables @@ -311,6 +317,19 @@ export class ObservableQuery< reobserveOptions.fetchPolicy = 'network-only'; } + if (__DEV__ && variables && hasOwnProperty.call(variables, "variables")) { + const queryDef = getQueryDefinition(this.options.query); + const vars = queryDef.variableDefinitions; + if (!vars || !vars.some(v => v.variable.name.value === "variables")) { + invariant.warn(`Called refetch(${ + JSON.stringify(variables) + }) for query ${ + queryDef.name?.value || JSON.stringify(queryDef) + }, which does not declare a $variables variable. +Did you mean to call refetch(variables) instead of refetch({ variables })?`); + } + } + if (variables && !equal(this.options.variables, variables)) { // Update the existing options with new variables reobserveOptions.variables = this.options.variables = { @@ -657,7 +676,7 @@ once, rather than every time you call fetchMore.`); // Disposable Concast fetches receive a shallow copy of this.options // (merged with newOptions), leaving this.options unmodified. ? compact(this.options, newOptions) - : Object.assign(this.options, compact(newOptions)); + : assign(this.options, compact(newOptions)); if (!useDisposableConcast) { // We can skip calling updatePolling if we're not changing this.options. diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts index 94bea64470a..4111121355b 100644 --- a/src/core/__tests__/ObservableQuery.ts +++ b/src/core/__tests__/ObservableQuery.ts @@ -1365,6 +1365,252 @@ describe('ObservableQuery', () => { }, }); }); + + describe("warnings about refetch({ variables })", () => { + itAsync("should warn if passed { variables } and query does not declare any variables", (resolve, reject) => { + const consoleWarnSpy = jest.spyOn(console, "warn"); + consoleWarnSpy.mockImplementation(() => {}); + + const queryWithoutVariables = gql` + query QueryWithoutVariables { + getVars { + __typename + name + } + } + `; + + function makeMock(...vars: string[]) { + const requestWithoutVariables = { + query: queryWithoutVariables, + variables: { + variables: vars, + }, + }; + + const resultWithVariables = { + data: { + getVars: vars.map(name => ({ + __typename: "Var", + name, + })), + }, + }; + + return { + request: requestWithoutVariables, + result: resultWithVariables, + }; + } + + const observableWithoutVariables: ObservableQuery = mockWatchQuery( + reject, + makeMock("a", "b", "c"), + makeMock("d", "e"), + ); + + subscribeAndCount(reject, observableWithoutVariables, (count, result) => { + expect(result.error).toBeUndefined(); + + if (count === 1) { + expect(result.loading).toBe(false); + expect(result.data).toEqual({ + getVars: [ + { __typename: "Var", name: "a" }, + { __typename: "Var", name: "b" }, + { __typename: "Var", name: "c" }, + ], + }); + + // It's a common mistake to call refetch({ variables }) when you meant + // to call refetch(variables). + observableWithoutVariables.refetch({ + variables: ["d", "e"], + }).catch(reject); + + } else if (count === 2) { + expect(result.loading).toBe(false); + expect(result.data).toEqual({ + getVars: [ + { __typename: "Var", name: "d" }, + { __typename: "Var", name: "e" }, + ], + }); + + expect(consoleWarnSpy).toHaveBeenCalledTimes(1); + expect(consoleWarnSpy).toHaveBeenCalledWith([ + 'Called refetch({"variables":["d","e"]}) for query QueryWithoutVariables, which does not declare a $variables variable.', + "Did you mean to call refetch(variables) instead of refetch({ variables })?", + ].join("\n")); + consoleWarnSpy.mockReset(); + + setTimeout(resolve, 10); + } else { + reject(`too many results (${count})`); + } + }); + }); + + itAsync("should warn if passed { variables } and query does not declare $variables", (resolve, reject) => { + const consoleWarnSpy = jest.spyOn(console, "warn"); + consoleWarnSpy.mockImplementation(() => {}); + + const queryWithVarsVar = gql` + query QueryWithVarsVar($vars: [String!]) { + getVars(variables: $vars) { + __typename + name + } + } + `; + + function makeMock(...vars: string[]) { + const requestWithVarsVar = { + query: queryWithVarsVar, + variables: { vars }, + }; + + const resultWithVarsVar = { + data: { + getVars: vars.map(name => ({ + __typename: "Var", + name, + })), + }, + }; + + return { + request: requestWithVarsVar, + result: resultWithVarsVar, + }; + } + + const observableWithVarsVar: ObservableQuery = mockWatchQuery( + reject, + makeMock("a", "b", "c"), + makeMock("d", "e"), + ); + + subscribeAndCount(error => { + expect(error.message).toMatch( + 'No more mocked responses for the query: query QueryWithVarsVar($vars: [String!])' + ); + }, observableWithVarsVar, (count, result) => { + expect(result.error).toBeUndefined(); + + if (count === 1) { + expect(result.loading).toBe(false); + expect(result.data).toEqual({ + getVars: [ + { __typename: "Var", name: "a" }, + { __typename: "Var", name: "b" }, + { __typename: "Var", name: "c" }, + ], + }); + + // It's a common mistake to call refetch({ variables }) when you meant + // to call refetch(variables). + observableWithVarsVar.refetch({ + variables: { vars: ["d", "e"] }, + }).then(result => { + reject(`unexpected result ${JSON.stringify(result)}; should have thrown`); + }, error => { + expect(error.message).toMatch( + 'No more mocked responses for the query: query QueryWithVarsVar($vars: [String!])' + ); + expect(consoleWarnSpy).toHaveBeenCalledTimes(1); + expect(consoleWarnSpy).toHaveBeenCalledWith([ + 'Called refetch({"variables":{"vars":["d","e"]}}) for query QueryWithVarsVar, which does not declare a $variables variable.', + "Did you mean to call refetch(variables) instead of refetch({ variables })?", + ].join("\n")); + consoleWarnSpy.mockReset(); + + setTimeout(resolve, 10); + }); + + } else { + reject(`one too many (${count}) results: ${JSON.stringify(result)}`); + } + }); + }); + + itAsync("should not warn if passed { variables } and query declares $variables", (resolve, reject) => { + const consoleWarnSpy = jest.spyOn(console, "warn"); + consoleWarnSpy.mockImplementation(() => {}); + + const queryWithVariablesVar = gql` + query QueryWithVariablesVar($variables: [String!]) { + getVars(variables: $variables) { + __typename + name + } + } + `; + + function makeMock(...variables: string[]) { + const requestWithVariablesVar = { + query: queryWithVariablesVar, + variables: { + variables, + }, + }; + + const resultWithVariablesVar = { + data: { + getVars: variables.map(name => ({ + __typename: "Var", + name, + })), + }, + }; + + return { + request: requestWithVariablesVar, + result: resultWithVariablesVar, + }; + } + + const observableWithVariablesVar: ObservableQuery = mockWatchQuery( + reject, + makeMock("a", "b", "c"), + makeMock("d", "e"), + ); + + subscribeAndCount(reject, observableWithVariablesVar, (count, result) => { + expect(result.error).toBeUndefined(); + if (count === 1) { + expect(result.loading).toBe(false); + expect(result.data).toEqual({ + getVars: [ + { __typename: "Var", name: "a" }, + { __typename: "Var", name: "b" }, + { __typename: "Var", name: "c" }, + ], + }); + + observableWithVariablesVar.refetch({ + variables: ["d", "e"], + }).catch(reject); + + } else if (count === 2) { + expect(result.loading).toBe(false); + expect(result.data).toEqual({ + getVars: [ + { __typename: "Var", name: "d" }, + { __typename: "Var", name: "e" }, + ], + }); + + expect(consoleWarnSpy).not.toHaveBeenCalled(); + consoleWarnSpy.mockReset(); + + setTimeout(resolve, 10); + } else { + reject(`too many results (${count})`); + } + }); + }); + }); }); describe('currentResult', () => {