Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Fix component stuck in loading state for network-only fetch policy #3126

Merged
merged 8 commits into from
Jun 21, 2019
Merged

Fix component stuck in loading state for network-only fetch policy #3126

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a9981b6
Add ability for Query to detect when lastResult is not accurate
jasonpaulos Jun 17, 2019
d4ce37e
Add regression test for #2899
jasonpaulos Jun 17, 2019
c2ff17c
Merge branch 'master' into fix-Query-lastResult
jasonpaulos Jun 17, 2019
f2f3b73
Merge branch 'master' into fix-Query-lastResult
hwillson Jun 19, 2019
5839fb9
Streamline interactions with Apollo Client
hwillson Jun 19, 2019
0fd0985
Code review tweaks
hwillson Jun 21, 2019
3adaecb
Merge branch 'master' into fix-Query-lastResult
hwillson Jun 21, 2019
c6428cf
Changelog update
hwillson Jun 21, 2019
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
75 changes: 22 additions & 53 deletions src/Query.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import ApolloClient, {
NetworkStatus,
FetchMoreOptions,
FetchMoreQueryOptions,
ApolloCurrentResult
} from 'apollo-client';
import { DocumentNode } from 'graphql';
import { ZenObservable } from 'zen-observable-ts';
Expand Down Expand Up @@ -115,7 +116,6 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
// only delete queryObservable when we unmount the component.
private queryObservable?: ObservableQuery<TData, TVariables> | null;
private querySubscription?: ZenObservable.Subscription;
private previousData: any = {};
private refetcherQueue?: {
args: any;
resolve: (value?: any | PromiseLike<any>) => void;
Expand All @@ -124,8 +124,7 @@ export default class Query<TData = any, TVariables = OperationVariables> extends

private hasMounted: boolean = false;
private operation?: IDocumentDefinition;
private lastResult: ApolloQueryResult<TData> | null = null;
private lastRenderIsDifferent: boolean = false;
private lastRenderedResult: ApolloQueryResult<TData> | null = null;

constructor(props: QueryProps<TData, TVariables>, context: QueryContext) {
super(props, context);
Expand Down Expand Up @@ -176,7 +175,7 @@ export default class Query<TData = any, TVariables = OperationVariables> 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)
Expand All @@ -188,6 +187,7 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
componentWillReceiveProps(nextProps: QueryProps<TData, TVariables>, nextContext: QueryContext) {
// the next render wants to skip
if (nextProps.skip && !this.props.skip) {
this.queryObservable!.resetLastResults();
this.removeQuerySubscription();
return;
}
Expand All @@ -202,11 +202,10 @@ export default class Query<TData = any, TVariables = OperationVariables> 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();
}

Expand All @@ -233,14 +232,7 @@ export default class Query<TData = any, TVariables = OperationVariables> extends

render(): React.ReactNode {
const { context } = this;
const finish = () => {
const result = this.getQueryResult();
this.lastRenderIsDifferent =
!this.lastResult ||
this.lastResult.loading !== result.loading ||
this.lastResult.networkStatus !== result.networkStatus;
return this.props.children(result);
};
const finish = () => this.props.children(this.getQueryResult());
if (context && context.renderPromises) {
return context.renderPromises.addQueryPromise(this, finish);
}
Expand Down Expand Up @@ -308,53 +300,28 @@ export default class Query<TData = any, TVariables = OperationVariables> 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<TData, TVariables> | 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.lastRenderIsDifferent &&
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 => {
Expand All @@ -368,8 +335,8 @@ export default class Query<TData = any, TVariables = OperationVariables> extends

private removeQuerySubscription = () => {
if (this.querySubscription) {
this.lastResult = this.queryObservable!.getLastResult();
this.querySubscription.unsubscribe();
delete this.lastRenderedResult;
delete this.querySubscription;
}
};
Expand All @@ -396,8 +363,6 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
// after a network based Query result has been received.
this.handleErrorOrCompleted();

this.lastRenderIsDifferent = false;

// Force a rerender that goes through shouldComponentUpdate.
if (this.hasMounted) this.forceUpdate();
};
Expand Down Expand Up @@ -429,7 +394,6 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
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;
Expand All @@ -443,8 +407,11 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
const { fetchPolicy } = this.queryObservable!.options;
Object.assign(data, { loading, networkStatus, error });

const previousData =
this.lastRenderedResult ? this.lastRenderedResult.data : {};

if (loading) {
Object.assign(data.data, this.previousData, currentResult.data);
Object.assign(data.data, previousData, currentResult.data);
hwillson marked this conversation as resolved.
Show resolved Hide resolved
} else if (error) {
Object.assign(data, {
data: (this.queryObservable!.getLastResult() || {}).data,
Expand All @@ -455,11 +422,12 @@ export default class Query<TData = any, TVariables = OperationVariables> 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;
data.data = previousData;
} else {
const { partialRefetch } = this.props;
if (
partialRefetch &&
typeof currentResult.data === 'object' &&
hwillson marked this conversation as resolved.
Show resolved Hide resolved
Object.keys(currentResult.data).length === 0 &&
partial &&
fetchPolicy !== 'cache-only'
Expand All @@ -474,11 +442,11 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
// this we'll attempt to refetch the `Query` data.
Object.assign(data, { loading: true, networkStatus: NetworkStatus.loading });
data.refetch();
this.lastRenderedResult = data;
return data;
}

Object.assign(data.data, currentResult.data);
this.previousData = currentResult.data;
}
}

Expand Down Expand Up @@ -524,6 +492,7 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
});

data.client = this.client;
this.lastRenderedResult = data;
return data;
};
}
28 changes: 18 additions & 10 deletions test/client/Query.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,14 +524,18 @@ describe('Query component', () => {

describe('props allow', () => {
it('custom fetch-policy', done => {
let count = 0;
const Component = () => (
<Query query={allPeopleQuery} fetchPolicy={'cache-only'}>
{result => {
catchAsyncError(done, () => {
expect(result.loading).toBeFalsy();
expect(result.networkStatus).toBe(NetworkStatus.ready);
done();
});
if (count === 0) {
hwillson marked this conversation as resolved.
Show resolved Hide resolved
catchAsyncError(done, () => {
expect(result.loading).toBeFalsy();
expect(result.networkStatus).toBe(NetworkStatus.ready);
done();
});
}
count += 1;
return null;
}}
</Query>
Expand All @@ -545,14 +549,18 @@ describe('Query component', () => {
});

it('default fetch-policy', done => {
let count = 0;
hwillson marked this conversation as resolved.
Show resolved Hide resolved
const Component = () => (
<Query query={allPeopleQuery}>
{result => {
catchAsyncError(done, () => {
expect(result.loading).toBeFalsy();
expect(result.networkStatus).toBe(NetworkStatus.ready);
done();
});
if (count === 0) {
catchAsyncError(done, () => {
hwillson marked this conversation as resolved.
Show resolved Hide resolved
expect(result.loading).toBeFalsy();
expect(result.networkStatus).toBe(NetworkStatus.ready);
done();
});
}
count += 1;
return null;
}}
</Query>
Expand Down
22 changes: 4 additions & 18 deletions test/client/graphql/mutations/recycled-queries.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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({
Expand All @@ -175,7 +160,7 @@ describe('graphql(mutation) update queries', () => {
],
});
break;
case 4:
case 3:
hwillson marked this conversation as resolved.
Show resolved Hide resolved
expect(stripSymbols(this.props.data!.todo_list)).toEqual({
id: '123',
title: 'how to apollo',
Expand All @@ -196,6 +181,7 @@ describe('graphql(mutation) update queries', () => {
default:
throw new Error('Rendered too many times');
}
queryRenderCount += 1;
} catch (error) {
reject(error);
}
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 0 additions & 12 deletions test/client/graphql/queries/lifecycle.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
hwillson marked this conversation as resolved.
Show resolved Hide resolved
{ 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 },
]);
});
Expand Down