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

Commit

Permalink
Prevent ObservableQuery component updates from being blocked
Browse files Browse the repository at this point in the history
When new data or errors are reported back through the
`ObservableQuery` subscription, `updateCurrentData` is called to
trigger a forced update of the parent component (to reflect the
new component state). Before triggering the forced update however,
`updateCurrentData` first checks to see if the component is
mounted (since we don't want to try to trigger a forced update on
a copmonent that isn't mounted). Unfortunately, a component
isn't marked as being mounted until the `afterExecute` method is
called via a `useEffect`. Under certain circumstances it's
possible for the `ObservableQuery` subscription to be
initialized and get data back before the `useEffect` has fired
and marked the component as mounted. This means when
`updateCurrentData` is triggered with valid updates, they're
sometimes blocked, and the component gets stuck in a permanent
loading state.

Due to other recent cleanup changes, we're now tracking the
destruction of the `ObservableQuery` subscription in a
`useEffect` within `useBaseQuery`. The subscription cleanup
is fired only when the component unmounts, and since we're
only calling `updateCurrentData` from within the subscription
callback, we don't need to have an extra `isMounted` check
before forcing an update. To this end, these changes get rid of
`updateCurrentData` completely. The `ObservableQuery`
subscription now always calls `forceUpdate` directly, and as
long as the component is mounted (tracked by React), it will
always force an update. This prevents race conditions between
data attempting to be updated, and the component not yet being
marked as mounted.

Fixes #3270.
Fixes #3299.
  • Loading branch information
hwillson committed Aug 5, 2019
1 parent 4483d9a commit 1171e75
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 9 deletions.
2 changes: 1 addition & 1 deletion packages/components/src/Query.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,6 @@ export namespace Query {
variables: PropTypes.object,
ssr: PropTypes.bool,
partialRefetch: PropTypes.bool,
returnPartialData: PropTypes.bool,
returnPartialData: PropTypes.bool
};
}
10 changes: 2 additions & 8 deletions packages/hooks/src/data/QueryData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -189,12 +189,6 @@ export class QueryData<TData, TVariables> extends OperationData {
return result;
}

private updateCurrentData() {
if (this.isMounted) {
this.forceUpdate();
}
}

private prepareObservableQueryOptions() {
this.verifyDocumentType(this.getOptions().query, DocumentType.Query);
const displayName = this.getOptions().displayName || 'Query';
Expand Down Expand Up @@ -286,12 +280,12 @@ export class QueryData<TData, TVariables> extends OperationData {
return;
}

this.updateCurrentData();
this.forceUpdate();
},
error: error => {
this.resubscribeToQuery();
if (!error.hasOwnProperty('graphQLErrors')) throw error;
this.updateCurrentData();
this.forceUpdate();
}
});
}
Expand Down

0 comments on commit 1171e75

Please sign in to comment.