Skip to content

Commit

Permalink
Stop delivering previous data in unrelated loading results. (#6566)
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 authored Jul 9, 2020
1 parent 0b2e92c commit a2c42ee
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 50 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,9 @@

- **[BREAKING]** The `QueryOptions`, `MutationOptions`, and `SubscriptionOptions` React Apollo interfaces have been renamed to `QueryDataOptions`, `MutationDataOptions`, and `SubscriptionDataOptions` (to avoid conflicting with similarly named and exported Apollo Client interfaces).

- **[BREAKING]** Results with `loading: true` will no longer redeliver previous data, though they may provide partial data from the cache, when available. <br/>
[@benjamn](https://github.com/benjamn) in [#6566](https://github.com/apollographql/apollo-client/pull/6566)

- **[BREAKING?]** Remove `fixPolyfills.ts`, except when bundling for React Native. If you have trouble with `Map` or `Set` operations due to frozen key objects in React Native, either update React Native to version 0.59.0 (or 0.61.x, if possible) or investigate why `fixPolyfills.native.js` is not included in your bundle. <br/>
[@benjamn](https://github.com/benjamn) in [#5962](https://github.com/apollographql/apollo-client/pull/5962)

Expand Down
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 a2c42ee

Please sign in to comment.