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.

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

Read from cache again in ObservableQuery#getCurrentResult. #5791

Merged
merged 3 commits into from
Jan 14, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented 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 (specifically, Promise-based) cache reads, but that form of asynchrony is no longer a goal of Apollo Client 3.0 or any planned future version (there will be other ways to deliver results asynchronously, without requiring every cache read to return a Promise), 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 benjamn added this to the Release 3.0 milestone Jan 14, 2020
@benjamn benjamn requested a review from hwillson January 14, 2020 21:11
@benjamn benjamn self-assigned this Jan 14, 2020
@benjamn benjamn force-pushed the reinstate-getCurrentResult-cache-reads branch from dff41e1 to 868aaf4 Compare January 14, 2020 21:25
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 benjamn force-pushed the reinstate-getCurrentResult-cache-reads branch from 868aaf4 to ecfb48f Compare January 14, 2020 22:02
Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Looks great @benjamn - thanks!

@hwillson
Copy link
Member

@benjamn I re-ran the tests to get them to pass; we have a timing issue buried in one of our polling tests, which I've made a note of and will address separately.

@jkonowitch
Copy link

jkonowitch commented Jan 15, 2020

I was just about to file a bug report. This PR fixes the issue, but wanted to document it here in case you had been unaware of this specific problem.

Using values in the cache as default arguments to queries (via the @client @export(as: "whatever") directives), did not work.

https://github.com/jkonowitch/react-apollo-error-template/tree/export-error

I set up a repro with the error template. I slighly extended the "server" schema so that the people query takes an argument - orderBy.

I then added activeSortOrder as a client side schema extension.

The following query did not return a result, whereas it should return the people entities, sorted correctly.

const ALL_PEOPLE = gql`
  query AllPeople($orderBy: SortOrder!) {
    activeSortOrder @client @export(as: "orderBy")
    people(orderBy: $orderBy) {
      id
      name
    }
  }
`;

This works as expected with beta.23 👏

@benjamn benjamn deleted the reinstate-getCurrentResult-cache-reads branch June 24, 2020 21:06
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants