Skip to content

Commit

Permalink
Stop delivering previous data in unrelated loading results.
Browse files Browse the repository at this point in the history 
Results with loading:true can still provide data from the cache, but they
should never provide data from a previous result, especially since the
previous result may have been derived from entirely different variables:
#6039 (comment)

This is potentially a breaking change for code that relied on result.data
not being undefined when result.loading is true, but the bugs that this
change will fix are much more serious than the inconvenience of checking
the truthiness of result.data before using it.

Fixes #6039, as verified by the reproduction provided by @davismariotti:
https://codesandbox.io/s/changing-variables-demo-obykj
  • Loading branch information
@benjamn
benjamn committed Jul 9, 2020
1 parent 0b2e92c commit f48583a
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 50 deletions.
19 changes: 16 additions & 3 deletions src/react/components/__tests__/client/Query.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1273,10 +1273,23 @@ describe('Query component', () => {
return (
<AllPeopleQuery query={query} variables={variables}>
{(result: any) => {
if (result.loading && count === 2) {
expect(stripSymbols(result.data)).toEqual(data1);
if (count === 0) {
expect(result.loading).toBe(true);
expect(result.data).toBeUndefined();
expect(result.networkStatus).toBe(NetworkStatus.loading);
} else if (count === 1) {
expect(result.loading).toBe(false);
expect(result.data).toEqual(data1);
expect(result.networkStatus).toBe(NetworkStatus.ready);
} else if (count === 2) {
expect(result.loading).toBe(true);
expect(result.data).toBeUndefined();
expect(result.networkStatus).toBe(NetworkStatus.setVariables);
} else if (count === 3) {
expect(result.loading).toBe(false);
expect(result.data).toEqual(data2);
expect(result.networkStatus).toBe(NetworkStatus.ready);
}

count++;
return null;
}}
Expand Down
17 changes: 4 additions & 13 deletions src/react/data/QueryData.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,8 @@ export class QueryData<TData, TVariables> extends OperationData {
} else if (this.currentObservable) {
// Fetch the current result (if any) from the store.
const currentResult = this.currentObservable.getCurrentResult();
const { loading, partial, networkStatus, errors } = currentResult;
let { error, data } = currentResult;
const { data, loading, partial, networkStatus, errors } = currentResult;
let { error } = currentResult;

// Until a set naming convention for networkError and graphQLErrors is
// decided upon, we map errors (graphQLErrors) to the error options.
Expand All @@ -361,22 +361,15 @@ export class QueryData<TData, TVariables> extends OperationData {

result = {
...result,
data,
loading,
networkStatus,
error,
called: true
};

if (loading) {
const previousData =
this.previousData.result && this.previousData.result.data;
result.data =
previousData && data
? {
...previousData,
...data
}
: previousData || data;
// Fall through without modifying result...
} else if (error) {
Object.assign(result, {
data: (this.currentObservable.getLastResult() || ({} as any))
Expand Down Expand Up @@ -406,8 +399,6 @@ export class QueryData<TData, TVariables> extends OperationData {
result.refetch();
return result;
}

result.data = data;
}
}

Expand Down
49 changes: 27 additions & 22 deletions src/react/hoc/__tests__/queries/lifecycle.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { ApolloClient } from '../../../../ApolloClient';
import { ApolloProvider } from '../../../context/ApolloProvider';
import { InMemoryCache as Cache } from '../../../../cache/inmemory/inMemoryCache';
import { mockSingleLink } from '../../../../utilities/testing/mocking/mockLink';
import { stripSymbols } from '../../../../utilities/testing/stripSymbols';
import { Query as QueryComponent } from '../../../components/Query';
import { graphql } from '../../graphql';
import { ChildProps } from '../../types';
Expand Down Expand Up @@ -56,12 +55,14 @@ describe('[queries] lifecycle', () => {
componentDidUpdate(prevProps: ChildProps<Vars, Data, Vars>) {
const { data } = this.props;
// loading is true, but data still there
if (count === 1 && data!.loading) {
expect(stripSymbols(data!.allPeople)).toEqual(data1.allPeople);
}
if (count === 1 && !data!.loading && prevProps.data!.loading) {
expect(stripSymbols(data!.allPeople)).toEqual(data2.allPeople);
done = true;
if (count === 1) {
if (data!.loading) {
expect(data!.allPeople).toBeUndefined();
} else {
expect(prevProps.data!.loading).toBe(true);
expect(data!.allPeople).toEqual(data2.allPeople);
done = true;
}
}
}
render() {
Expand Down Expand Up @@ -197,12 +198,14 @@ describe('[queries] lifecycle', () => {
componentDidUpdate(prevProps: ChildProps<Vars, Data, Vars>) {
const { data } = this.props;
// loading is true, but data still there
if (count === 1 && data!.loading) {
expect(stripSymbols(data!.allPeople)).toEqual(data1.allPeople);
}
if (count === 1 && !data!.loading && prevProps.data!.loading) {
expect(stripSymbols(data!.allPeople)).toEqual(data2.allPeople);
done = true;
if (count === 1) {
if (data!.loading) {
expect(data!.allPeople).toBeUndefined();
} else {
expect(prevProps.data!.loading).toBe(true);
expect(data!.allPeople).toEqual(data2.allPeople);
done = true;
}
}
}
render() {
Expand Down Expand Up @@ -272,12 +275,14 @@ describe('[queries] lifecycle', () => {
componentDidUpdate(prevProps: ChildProps<Vars, Data, Vars>) {
const { data } = this.props;
// loading is true, but data still there
if (count === 1 && data!.loading) {
expect(stripSymbols(data!.allPeople)).toEqual(data1.allPeople);
}
if (count === 1 && !data!.loading && prevProps.data!.loading) {
expect(stripSymbols(data!.allPeople)).toEqual(data2.allPeople);
done = true;
if (count === 1) {
if (data!.loading) {
expect(data!.allPeople).toBeUndefined();
} else {
expect(prevProps.data!.loading).toBe(true);
expect(data!.allPeople).toEqual(data2.allPeople);
done = true;
}
}
}
render() {
Expand Down Expand Up @@ -352,21 +357,21 @@ describe('[queries] lifecycle', () => {
if (count === 1) {
expect(props.foo).toEqual(42);
expect(props.data!.loading).toEqual(false);
expect(stripSymbols(props.data!.allPeople)).toEqual(
expect(props.data!.allPeople).toEqual(
data1.allPeople
);
props.changeState();
} else if (count === 2) {
expect(props.foo).toEqual(43);
expect(props.data!.loading).toEqual(false);
expect(stripSymbols(props.data!.allPeople)).toEqual(
expect(props.data!.allPeople).toEqual(
data1.allPeople
);
props.data!.refetch();
} else if (count === 3) {
expect(props.foo).toEqual(43);
expect(props.data!.loading).toEqual(false);
expect(stripSymbols(props.data!.allPeople)).toEqual(
expect(props.data!.allPeople).toEqual(
data2.allPeople
);
}
Expand Down
24 changes: 12 additions & 12 deletions src/react/hoc/__tests__/queries/loading.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { ApolloClient } from '../../../../ApolloClient';
import { ApolloProvider } from '../../../context/ApolloProvider';
import { InMemoryCache as Cache } from '../../../../cache/inmemory/inMemoryCache';
import { mockSingleLink } from '../../../../utilities/testing/mocking/mockLink';
import { stripSymbols } from '../../../../utilities/testing/stripSymbols';
import { graphql } from '../../graphql';
import { ChildProps } from '../../types';
import { itAsync } from '../../../../utilities/testing/itAsync';
Expand Down Expand Up @@ -185,15 +184,16 @@ describe('[queries] loading', () => {
componentDidUpdate(prevProps: ChildProps<Vars, Data, Vars>) {
const { data } = this.props;
// variables changed, new query is loading, but old data is still there
if (count === 1 && data!.loading) {
expect(data!.networkStatus).toBe(2);
expect(stripSymbols(data!.allPeople)).toEqual(data1.allPeople);
}
// query with new variables is loaded
if (count === 1 && !data!.loading && prevProps.data!.loading) {
expect(data!.networkStatus).toBe(7);
expect(stripSymbols(data!.allPeople)).toEqual(data2.allPeople);
done = true;
if (count === 1) {
if (data!.loading) {
expect(data!.networkStatus).toBe(2);
expect(data!.allPeople).toBeUndefined();
} else {
expect(prevProps.data!.loading).toBe(true);
expect(data!.networkStatus).toBe(7);
expect(data!.allPeople).toEqual(data2.allPeople);
done = true;
}
}
}
render() {
Expand Down Expand Up @@ -268,12 +268,12 @@ describe('[queries] loading', () => {
case 1:
expect(data!.loading).toBeTruthy();
expect(data!.networkStatus).toBe(4);
expect(stripSymbols(data!.allPeople)).toEqual(data!.allPeople);
expect(data!.allPeople).toEqual(data!.allPeople);
break;
case 2:
expect(data!.loading).toBeFalsy();
expect(data!.networkStatus).toBe(7);
expect(stripSymbols(data!.allPeople)).toEqual(data2.allPeople);
expect(data!.allPeople).toEqual(data2.allPeople);
break;
default:
reject(new Error('Too many props updates'));
Expand Down

0 comments on commit f48583a

Please sign in to comment.