Avoid reading from cache in ObservableQuery#getCurrentResult. #5565
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When fetchQuery decides not to make a network request (!shouldFetch), we can safely assume broadcastQueries will be called later in the fetchQuery method, after the cache has been updated. Calling broadcastQueries unconditionally in the middle of the fetchQuery method (before the fetchRequest call) guarantees an unnecessary extra broadcast when shouldFetch is false, which can lead to multiple UI renders.
Since getCurrentResult must return its result synchronously, reading from the cache in this method prevents us from making cache reads asynchronous in the future. This may be a breaking change for code that attempts to read cache data from an ObservableQuery immediately after calling watchQuery, but that's a behavior that should never have been enforced by our test suite in the first place.
Since we are no longer reading from the cache in getCurrentResult, and the cache was the source of the partial boolean, and all tested occurrences of the partial boolean were false anyway, we can safely remove the partial field from ApolloCurrentQueryResult, which is a small step towards not needing ApolloCurrentQueryResult in addition to ApolloQueryResult.
benjamn
changed the title
benjamn
added a commit
that referenced
this pull request
Nov 12, 2019
benjamn
added a commit
that referenced
this pull request
Nov 21, 2019
In commit 6e0135d (part of PR #5565) I added a this.queryStore.initQuery call to the QueryManager#watchQuery method, to ensure the queryStore has information about watched queries as early as possible. However, I mistakenly copied the isPoll option value from the initQuery call site in fetchQuery, which caused some watched queries (those that use polling) to have NetworkStatus.poll instead of NetworkStatus.loading before any network activity had occurred. This commit fixes that problem by always passing isPoll:false in watchQuery, regardless of whether options.pollInterval is defined.
hwillson
added a commit
that referenced
this pull request
Nov 30, 2019
The changes made in #5565 mean that `QueryData#fetchData` should no longer directly rely on `ObservableQuery#getCurrentResult` or `ObservableQuery#result`. This commit adjusts `fetchData` such that it returns a new Promise that resolves when the `ObservableQuery` subscription receives new data, re-using the same `startQuerySubscription` business rules that non-SSR renders adhere to. This ensures that the `queryPromises` (in `react/ssr/RenderPromises`) stored for SSR resolve properly after new data is received.
hwillson
added a commit
that referenced
this pull request
Nov 30, 2019
The changes made in #5565 mean that `QueryData#fetchData` should no longer directly rely on `ObservableQuery#getCurrentResult` or `ObservableQuery#result`. This commit adjusts `fetchData` such that it returns a new Promise that resolves when the `ObservableQuery` subscription receives new data, re-using the same `startQuerySubscription` business rules that non-SSR renders adhere to. This ensures that the `queryPromises` (in `react/ssr/RenderPromises`) stored for SSR resolve properly after new data is received.
hwillson
added a commit
to apollographql/react-apollo
that referenced
this pull request
Nov 30, 2019
Since `ObservableQuery#getCurrentResult` no longer reads from the
cache directly, several React Apollo tests need to be updated to
reflect new loading states that can occur if `getCurrentResult`
doesn't yet have any data to return when it's called.
Also, the previous SSR testing pattern of:
```
return getDataFromTree(app).then(() => {
const markup = ReactDOM.renderToString(app);
expect(markup).toMatch(/James/);
});
```
will no longer work, as `ReactDOM.renderToString(app)` will
just return the initial loading state of the component
under test. Instead, we can leverage the markup returned
when `getDataFromTree`'s Promise resolves:
return getDataFromTree(app).then(markup => {
expect(markup).toMatch(/James/);
});
cc @benjamn
hwillson
added a commit
that referenced
this pull request
Dec 2, 2019
* Adjust `fetchData` to accommodate #5565 The changes made in #5565 mean that `QueryData#fetchData` should no longer directly rely on `ObservableQuery#getCurrentResult` or `ObservableQuery#result`. This commit adjusts `fetchData` such that it returns a new Promise that resolves when the `ObservableQuery` subscription receives new data, re-using the same `startQuerySubscription` business rules that non-SSR renders adhere to. This ensures that the `queryPromises` (in `react/ssr/RenderPromises`) stored for SSR resolve properly after new data is received. * Remove no longer valid Promise generic
benjamn
added a commit
that referenced
this pull request
Jan 14, 2020
This partially reverts two commits from PR #5565, preserving related improvements that have happened in the meantime: * Revert "Remove partial field from ApolloCurrentQueryResult type." This reverts commit ca2cef6. * Revert "Stop reading from cache in ObservableQuery#getCurrentResult." This reverts commit 01376a3. One of the motivations behind the original changes was to lay the foundations for eventually supporting asynchronous (Promise-based) cache reads, but that form of asynchrony is no longer a goal of Apollo Client 3.0 or any planned future version, so there's not as much benefit to weakening getCurrentResult in this way. On the other hand, allowing getCurrentResult to read synchronously from the cache ensures that custom field read functions can return new results immediately, which helps with issues like #5782. Immediate cache reads have also been the behavior of Apollo Client for much longer than #5565, which makes this change appealing from an ease-of-upgrading perspective.
benjamn
added a commit
that referenced
this pull request
Jan 14, 2020
This partially reverts two commits from PR #5565, preserving related improvements that have happened in the meantime: * Revert "Remove partial field from ApolloCurrentQueryResult type." This reverts commit ca2cef6. * Revert "Stop reading from cache in ObservableQuery#getCurrentResult." This reverts commit 01376a3. One of the motivations behind the original changes was to lay the foundations for eventually supporting asynchronous (Promise-based) cache reads, but that form of asynchrony is no longer a goal of Apollo Client 3.0 or any planned future version, so there's not as much benefit to weakening getCurrentResult in this way. On the other hand, allowing getCurrentResult to read synchronously from the cache ensures that custom field read functions can return new results immediately, which helps with issues like #5782. Immediate cache reads have also been the behavior of Apollo Client for much longer than #5565, which makes this change appealing from an ease-of-upgrading perspective.
benjamn
added a commit
that referenced
this pull request
Jan 14, 2020
This partially reverts two commits from PR #5565, preserving related improvements that have happened in the meantime: * Revert "Remove partial field from ApolloCurrentQueryResult type." This reverts commit ca2cef6. * Revert "Stop reading from cache in ObservableQuery#getCurrentResult." This reverts commit 01376a3. One of the motivations behind the original changes was to lay the foundations for eventually supporting asynchronous (Promise-based) cache reads, but that form of asynchrony is no longer a goal of Apollo Client 3.0 or any planned future version, so there's not as much benefit to weakening getCurrentResult in this way. On the other hand, allowing getCurrentResult to read synchronously from the cache ensures that custom field read functions can return new results immediately, which helps with issues like #5782. Immediate cache reads have also been the behavior of Apollo Client for much longer than #5565, which makes this change appealing from an ease-of-upgrading perspective.
benjamn
added a commit
that referenced
this pull request
Jan 14, 2020
This partially reverts two commits from PR #5565, preserving related improvements that have happened in the meantime: * Revert "Remove partial field from ApolloCurrentQueryResult type." This reverts commit ca2cef6. * Revert "Stop reading from cache in ObservableQuery#getCurrentResult." This reverts commit 01376a3. One of the motivations behind the original changes was to lay the foundations for eventually supporting asynchronous (Promise-based) cache reads, but that form of asynchrony is no longer a goal of Apollo Client 3.0 or any planned future version, so there's not as much benefit to weakening getCurrentResult in this way. On the other hand, allowing getCurrentResult to read synchronously from the cache ensures that custom field read functions can return new results immediately, which helps with issues like #5782. Immediate cache reads have also been the behavior of Apollo Client for much longer than #5565, which makes this change appealing from an ease-of-upgrading perspective.
benjamn
added a commit
that referenced
this pull request
Jan 14, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As an exploration of potential improvements after Apollo Client 3.0, I recently attempted to make
InMemoryCachereads asynchronous, so that custom fieldreadfunctions could return a promise, or perhaps an async iterator, which would ultimately allow us to remove our current local state implementation, which would be a significant bundle size savings.The bad news: I had to update a lot of tests (no surprise there) in ways that felt too drastic for the Apollo Client 3.x release line, so I think we should postpone the idea of async cache reads until at least Apollo Client 4.0. Oh well!
The silver lining: I made a series of changes to prepare for async cache reads that make lots of sense right now. Hence, the contents of this PR.
The most important change is
Stop reading from cache in ObservableQuery#getCurrentResult.(01376a3), which makes thegetCurrentResultfunction return whatever result it most recently received, without falling back to reading from the cache. This change was originally intended to enable async cache reads while keepinggetCurrentResultsynchronous (which is essential for React rendering, even with Suspense), but I believe this change will also prevent a whole class of inconsistencies due to reading from the cache at arbitrary times, rather than trusting Apollo Client to deliver results from the cache at appropriate times.