diff --git a/Changelog.md b/Changelog.md
index 40309c90ff..52f57fee2f 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -31,9 +31,11 @@
responses couldn't be handled). The `Query` component can now handle an
error in a response, then continue to handle a valid response afterwards.
[@hwillson](https://github.com/hwillson) in [#3107](https://github.com/apollographql/react-apollo/pull/3107)
-- Reorder `Subscription` component code to avoid setting state on unmounted
+- Reorder `Subscription` component code to avoid setting state on unmounted
component.
[@jasonpaulos](https://github.com/jasonpaulos) in [#3139](https://github.com/apollographql/react-apollo/pull/3139)
+- Fix component stuck in `loading` state for `network-only` fetch policy.
+ [@jasonpaulos](https://github.com/jasonpaulos) in [#3126](https://github.com/apollographql/react-apollo/pull/3126)
## 2.5.6 (2019-05-22)
diff --git a/src/Query.tsx b/src/Query.tsx
index 2fd7642abb..7574b1a784 100644
--- a/src/Query.tsx
+++ b/src/Query.tsx
@@ -7,6 +7,7 @@ import ApolloClient, {
NetworkStatus,
FetchMoreOptions,
FetchMoreQueryOptions,
+ ApolloCurrentResult
} from 'apollo-client';
import { DocumentNode } from 'graphql';
import { ZenObservable } from 'zen-observable-ts';
@@ -115,7 +116,6 @@ export default class Query extends
// only delete queryObservable when we unmount the component.
private queryObservable?: ObservableQuery | null;
private querySubscription?: ZenObservable.Subscription;
- private previousData: any = {};
private refetcherQueue?: {
args: any;
resolve: (value?: any | PromiseLike) => void;
@@ -124,7 +124,7 @@ export default class Query extends
private hasMounted: boolean = false;
private operation?: IDocumentDefinition;
- private lastResult: ApolloQueryResult | null = null;
+ private lastRenderedResult: ApolloQueryResult | null = null;
constructor(props: QueryProps, context: QueryContext) {
super(props, context);
@@ -175,7 +175,7 @@ export default class Query extends
this.hasMounted = true;
if (this.props.skip) return;
- this.startQuerySubscription(true);
+ this.startQuerySubscription();
if (this.refetcherQueue) {
const { args, resolve, reject } = this.refetcherQueue;
this.queryObservable!.refetch(args)
@@ -187,6 +187,7 @@ export default class Query extends
componentWillReceiveProps(nextProps: QueryProps, nextContext: QueryContext) {
// the next render wants to skip
if (nextProps.skip && !this.props.skip) {
+ this.queryObservable!.resetLastResults();
this.removeQuerySubscription();
return;
}
@@ -201,11 +202,10 @@ export default class Query extends
this.client = nextClient;
this.removeQuerySubscription();
this.queryObservable = null;
- this.previousData = {};
- this.updateQuery(nextProps);
}
if (this.props.query !== nextProps.query) {
+ this.queryObservable!.resetLastResults();
this.removeQuerySubscription();
}
@@ -300,52 +300,28 @@ export default class Query extends
.catch(() => null);
}
- private startQuerySubscription = (justMounted: boolean = false) => {
+ private startQuerySubscription = () => {
// When the `Query` component receives new props, or when we explicitly
// re-subscribe to a query using `resubscribeToQuery`, we start a new
// subscription in this method. To avoid un-necessary re-renders when
// receiving new props or re-subscribing, we track the full last
// observable result so it can be compared against incoming new data.
// We only trigger a re-render if the incoming result is different than
- // the stored `lastResult`.
- //
- // It's important to note that when a component is first mounted,
- // the `startQuerySubscription` method is also triggered. During a first
- // mount, we don't want to store or use the last result, as we always
- // need the first render to happen, even if there was a previous last
- // result (which can happen when the same component is mounted, unmounted,
- // and mounted again).
- if (!justMounted) {
- this.lastResult = this.queryObservable!.getLastResult();
- }
+ // the stored `lastRenderedResult`.
if (this.querySubscription) return;
- // store the initial renders worth of result
- let initial: QueryResult | undefined = this.getQueryResult();
-
this.querySubscription = this.queryObservable!.subscribe({
- next: ({ loading, networkStatus, data }) => {
- // to prevent a quick second render from the subscriber
- // we compare to see if the original started finished (from cache) and is unchanged
- if (initial && initial.networkStatus === 7 && shallowEqual(initial.data, data)) {
- initial = undefined;
- return;
- }
-
+ next: (result) => {
if (
- this.lastResult &&
- this.lastResult.loading === loading &&
- this.lastResult.networkStatus === networkStatus &&
- shallowEqual(this.lastResult.data, data)
+ this.lastRenderedResult &&
+ this.lastRenderedResult.loading === result.loading &&
+ this.lastRenderedResult.networkStatus === result.networkStatus &&
+ shallowEqual(this.lastRenderedResult.data, result.data)
) {
return;
}
- initial = undefined;
- if (this.lastResult) {
- this.lastResult = this.queryObservable!.getLastResult();
- }
this.updateCurrentData();
},
error: error => {
@@ -359,8 +335,8 @@ export default class Query extends
private removeQuerySubscription = () => {
if (this.querySubscription) {
- this.lastResult = this.queryObservable!.getLastResult();
this.querySubscription.unsubscribe();
+ delete this.lastRenderedResult;
delete this.querySubscription;
}
};
@@ -403,22 +379,21 @@ export default class Query extends
}
private getQueryResult = (): QueryResult => {
- let data = { data: Object.create(null) as TData } as any;
+ let result = { data: Object.create(null) as TData } as any;
// Attach bound methods
- Object.assign(data, observableQueryFields(this.queryObservable!));
+ Object.assign(result, observableQueryFields(this.queryObservable!));
// When skipping a query (ie. we're not querying for data but still want
// to render children), make sure the `data` is cleared out and
// `loading` is set to `false` (since we aren't loading anything).
if (this.props.skip) {
- data = {
- ...data,
+ result = {
+ ...result,
data: undefined,
error: undefined,
loading: false,
};
} else {
- // Fetch the current result (if any) from the store.
const currentResult = this.queryObservable!.currentResult();
const { loading, partial, networkStatus, errors } = currentResult;
let { error } = currentResult;
@@ -430,12 +405,15 @@ export default class Query extends
}
const { fetchPolicy } = this.queryObservable!.options;
- Object.assign(data, { loading, networkStatus, error });
+ Object.assign(result, { loading, networkStatus, error });
+
+ const previousData =
+ this.lastRenderedResult ? this.lastRenderedResult.data : {};
if (loading) {
- Object.assign(data.data, this.previousData, currentResult.data);
+ Object.assign(result.data, previousData, currentResult.data);
} else if (error) {
- Object.assign(data, {
+ Object.assign(result, {
data: (this.queryObservable!.getLastResult() || {}).data,
});
} else if (
@@ -444,11 +422,13 @@ export default class Query extends
) {
// Make sure data pulled in by a `no-cache` query is preserved
// when the components parent tree is re-rendered.
- data.data = this.previousData;
+ result.data = previousData;
} else {
const { partialRefetch } = this.props;
if (
partialRefetch &&
+ currentResult.data !== null &&
+ typeof currentResult.data === 'object' &&
Object.keys(currentResult.data).length === 0 &&
partial &&
fetchPolicy !== 'cache-only'
@@ -461,13 +441,13 @@ export default class Query extends
// the original `Query` component are expecting certain data values to
// exist, and they're all of a sudden stripped away. To help avoid
// this we'll attempt to refetch the `Query` data.
- Object.assign(data, { loading: true, networkStatus: NetworkStatus.loading });
- data.refetch();
- return data;
+ Object.assign(result, { loading: true, networkStatus: NetworkStatus.loading });
+ result.refetch();
+ this.lastRenderedResult = result;
+ return result;
}
- Object.assign(data.data, currentResult.data);
- this.previousData = currentResult.data;
+ Object.assign(result.data, currentResult.data);
}
}
@@ -491,9 +471,9 @@ export default class Query extends
// always hit the network with refetch, since the components data will be
// updated and a network request is not currently active.
if (!this.querySubscription) {
- const oldRefetch = (data as QueryControls).refetch;
+ const oldRefetch = (result as QueryControls).refetch;
- (data as QueryControls).refetch = args => {
+ (result as QueryControls).refetch = args => {
if (this.querySubscription) {
return oldRefetch(args);
} else {
@@ -512,7 +492,8 @@ export default class Query extends
this.queryObservable!.resetQueryStoreErrors();
});
- data.client = this.client;
- return data;
+ result.client = this.client;
+ this.lastRenderedResult = result;
+ return result;
};
}
diff --git a/test/client/Query.test.tsx b/test/client/Query.test.tsx
index ddf2a376a2..fb81fd22b9 100644
--- a/test/client/Query.test.tsx
+++ b/test/client/Query.test.tsx
@@ -524,14 +524,18 @@ describe('Query component', () => {
describe('props allow', () => {
it('custom fetch-policy', done => {
+ let count = 0;
const Component = () => (
{result => {
- catchAsyncError(done, () => {
- expect(result.loading).toBeFalsy();
- expect(result.networkStatus).toBe(NetworkStatus.ready);
- done();
- });
+ if (count === 0) {
+ catchAsyncError(done, () => {
+ expect(result.loading).toBeFalsy();
+ expect(result.networkStatus).toBe(NetworkStatus.ready);
+ done();
+ });
+ }
+ count += 1;
return null;
}}
@@ -545,14 +549,18 @@ describe('Query component', () => {
});
it('default fetch-policy', done => {
+ let count = 0;
const Component = () => (
{result => {
- catchAsyncError(done, () => {
- expect(result.loading).toBeFalsy();
- expect(result.networkStatus).toBe(NetworkStatus.ready);
- done();
- });
+ if (count === 0) {
+ catchAsyncError(done, () => {
+ expect(result.loading).toBeFalsy();
+ expect(result.networkStatus).toBe(NetworkStatus.ready);
+ done();
+ });
+ }
+ count += 1;
return null;
}}
diff --git a/test/client/graphql/mutations/recycled-queries.test.tsx b/test/client/graphql/mutations/recycled-queries.test.tsx
index 5366b2412f..02924181c1 100644
--- a/test/client/graphql/mutations/recycled-queries.test.tsx
+++ b/test/client/graphql/mutations/recycled-queries.test.tsx
@@ -128,7 +128,7 @@ describe('graphql(mutation) update queries', () => {
render() {
try {
- switch (queryRenderCount++) {
+ switch (queryRenderCount) {
case 0:
expect(this.props.data!.loading).toBeTruthy();
expect(this.props.data!.todo_list).toBeFalsy();
@@ -141,21 +141,6 @@ describe('graphql(mutation) update queries', () => {
});
break;
case 2:
- expect(queryMountCount).toBe(1);
- expect(queryUnmountCount).toBe(0);
- expect(stripSymbols(this.props.data!.todo_list)).toEqual({
- id: '123',
- title: 'how to apollo',
- tasks: [
- {
- id: '99',
- text: 'This one was created with a mutation.',
- completed: true,
- },
- ],
- });
- break;
- case 3:
expect(queryMountCount).toBe(2);
expect(queryUnmountCount).toBe(1);
expect(stripSymbols(this.props.data!.todo_list)).toEqual({
@@ -175,7 +160,7 @@ describe('graphql(mutation) update queries', () => {
],
});
break;
- case 4:
+ case 3:
expect(stripSymbols(this.props.data!.todo_list)).toEqual({
id: '123',
title: 'how to apollo',
@@ -196,6 +181,7 @@ describe('graphql(mutation) update queries', () => {
default:
throw new Error('Rendered too many times');
}
+ queryRenderCount += 1;
} catch (error) {
reject(error);
}
@@ -247,7 +233,7 @@ describe('graphql(mutation) update queries', () => {
expect(todoUpdateQueryCount).toBe(2);
expect(queryMountCount).toBe(2);
expect(queryUnmountCount).toBe(2);
- expect(queryRenderCount).toBe(4);
+ expect(queryRenderCount).toBe(3);
resolve();
} catch (error) {
reject(error);
diff --git a/test/client/graphql/queries/lifecycle.test.tsx b/test/client/graphql/queries/lifecycle.test.tsx
index b244ce6827..eaf733acc2 100644
--- a/test/client/graphql/queries/lifecycle.test.tsx
+++ b/test/client/graphql/queries/lifecycle.test.tsx
@@ -581,20 +581,8 @@ describe('[queries] lifecycle', () => {
{ loading: false, a: 7, b: 8, c: 9 },
// Load 6
-
- // The first render is caused by the component having its state updated
- // when switching the client. The 2nd and 3rd renders are caused by the
- // component re-subscribing to get data from Apollo Client.
- { loading: false, a: 1, b: 2, c: 3 },
- { loading: false, a: 1, b: 2, c: 3 },
{ loading: false, a: 1, b: 2, c: 3 },
-
- { loading: false, a: 4, b: 5, c: 6 },
{ loading: false, a: 4, b: 5, c: 6 },
- { loading: false, a: 4, b: 5, c: 6 },
-
- { loading: false, a: 7, b: 8, c: 9 },
- { loading: false, a: 7, b: 8, c: 9 },
{ loading: false, a: 7, b: 8, c: 9 },
]);
});
diff --git a/test/client/graphql/queries/loading.test.tsx b/test/client/graphql/queries/loading.test.tsx
index 88d0254419..18b047f68e 100644
--- a/test/client/graphql/queries/loading.test.tsx
+++ b/test/client/graphql/queries/loading.test.tsx
@@ -614,4 +614,111 @@ describe('[queries] loading', () => {
,
);
});
+
+ it('correctly sets loading state on component with changed variables, unchanged result, and network-only', done => {
+ const query: DocumentNode = gql`
+ query remount($first: Int) {
+ allPeople(first: $first) {
+ people {
+ name
+ }
+ }
+ }
+ `;
+ interface Data {
+ allPeople: {
+ people: { name: string }[];
+ };
+ }
+
+ const data = { allPeople: { people: [{ name: 'Luke Skywalker' }] } };
+ const variables = { first: 1 };
+ const variables2 = { first: 2 };
+
+ type Vars = typeof variables;
+ const link = mockSingleLink(
+ { request: { query, variables }, result: { data }, delay: 10 },
+ {
+ request: { query, variables: variables2 },
+ result: { data },
+ delay: 10,
+ },
+ );
+ const client = new ApolloClient({
+ link,
+ cache: new Cache({ addTypename: false }),
+ });
+ let count = 0;
+
+ interface Props extends Vars {
+ setFirst: (first: number) => void;
+ }
+
+ const connect = (component: React.ComponentType): React.ComponentType<{}> => {
+ return class extends React.Component<{}, { first: number }> {
+ constructor(props: {}) {
+ super(props);
+
+ this.state = {
+ first: 1,
+ };
+ this.setFirst = this.setFirst.bind(this);
+ }
+
+ setFirst(first: number) {
+ this.setState({ first });
+ }
+
+ render() {
+ return React.createElement(component, {
+ first: this.state.first,
+ setFirst: this.setFirst,
+ });
+ }
+ };
+ };
+
+ const Container = connect(
+ graphql(query, {
+ options: ({ first }) => ({ variables: { first }, fetchPolicy: 'network-only' })
+ })(
+ class extends React.Component> {
+ componentWillReceiveProps(props: ChildProps) {
+ if (count === 0) {
+ expect(props.data!.loading).toBeTruthy();
+ }
+
+ if (count === 1) {
+ expect(props.data!.loading).toBeFalsy(); // has initial data
+ expect(props.data!.allPeople).toEqual(data.allPeople);
+ setTimeout(() => {
+ this.props.setFirst(2);
+ }, 5);
+ }
+
+ if (count === 2) {
+ expect(props.data!.loading).toBeTruthy(); // on variables change
+ }
+
+ if (count === 3) {
+ // new data after fetch
+ expect(props.data!.loading).toBeFalsy();
+ expect(props.data!.allPeople).toEqual(data.allPeople);
+ done();
+ }
+ count++;
+ }
+ render() {
+ return null;
+ }
+ },
+ ),
+ );
+
+ renderer.create(
+
+
+ ,
+ );
+ });
});