Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop delivering previous data in unrelated loading results. #6566

Merged
merged 2 commits into from
Jul 9, 2020
Merged

Stop delivering previous data in unrelated loading results. #6566

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f48583a
Stop delivering previous data in unrelated loading results.
benjamn Jul 9, 2020
39310e1
Mention PR #6566 in CHANGELOG.md.
benjamn Jul 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions src/react/components/__tests__/client/Query.test.tsx
View file
Open in desktop
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
Open in desktop
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...
Comment on lines 371 to +372
Copy link
Member Author

@benjamn benjamn Jul 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we could default result.data to an empty object here, if we think that will help reduce the risk of this (admittedly late-breaking) change. However, that logic would only apply for React code using QueryData class.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storing empty objects in data has gotten us into trouble in the past (as outlined in apollographql/react-apollo#3388), so I'm all for keeping this as is.

} 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
Open in desktop
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
Open in desktop
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