apollographql / react-apollo

:recycle: React integration for Apollo Client
https://www.apollographql.com/docs/react/
MIT License
6.85k stars 787 forks source link

graphql HOC is not pre-fetch friendly #1440

Closed PolGuixe closed 5 years ago

PolGuixe commented 6 years ago

Intended outcome:

graphql HOC should return data from the client store if the data is in the client memory.

Actual outcome:

A network query is performed for a particular query if the query is new, even if the data is already in the client.

The scenario is as follows:

  1. Perform a query to prefetch a series of documents, with the goal to improve navigation performance and UX.
  2. After the serves responds to the query, all the data is already in the client store.
  3. User tries to access to a particular document.
  4. graphql HOC tries to perform a network query to retrieve information that is already in the store.
  5. User waits until the new query is resolved.

Right now we have built a work around using readFragment, however this is something that may be could be added to the graphql HoC.

More details discussed here https://github.com/VulcanJS/Vulcan/issues/1782#issuecomment-352615468

How to reproduce the issue:

Steps:

  1. Define a query to pre-fetch a list of documents. Make sure it's querying for all the fields required in the query defined in the graphql HOC.
  2. Execute the query defined above before executing the graphql HOC.
  3. Wait until the pre-fetch query payload is returned.
  4. Execute the graphql HOC -unsure the document you are querying is part of the pre-fetched documents-.

Version

SachaG commented 6 years ago

So to simplify the issue, correct me if I'm wrong but you're saying that if you have two different queries asking for the same data, the second query will not use the cache even though the data is already available on the client?

Also it's a small detail but I don't think these data-loading issues are related to the graphql HoC specifically, it's more about how Apollo works generally (whether you use the HoC or not).

PolGuixe commented 6 years ago

@SachaG kind of yes. The first query is a broader one - a list of documents with their fields -. The second query is narrower, with the same fields and a subset of data of the first query - a document of the list with their fields-.

(If this is not the right place, please point where this issue should be raised. 😉)

rosskevin commented 6 years ago

I don't know that this is related to the graphql hoc (or the new Query component), or apollo-client itself. I would ping our slack channel.

Adding a test here that proves your case is always helpful.

SachaG commented 6 years ago

Before adding a test, maybe we could get a confirmation of what the expected behavior is in that situation?

i.e. if you have two different queries, one that queries for a list of documents and one that queries for a single document from within that list, should you expect the single document to be loaded from cached data or not?

rosskevin commented 6 years ago

Now that i have spent more time in the code, that sounds definitely like a question for apollo-client. I am no expert but logically I expect your follow up query, since it is a subset, to be cached. react-apollo is mostly a thin veneer to convey queries to apollo-client.

You might peruse the tests in apollo-client to see if there is a case matching or near your situation. I would file an issue there. Please link to this issue so we have a history.

ShockiTV commented 6 years ago

There are no followup queries. Each query is different and you dont know what it will return. Even with different variables you dont know what it will return so you really need to run it over backend.

Example you have getBooks query, you run it with type: "drama" and than type: "comics". How do you know if they overlap without contacting backend? Same scenario with getDramaBooks and getComicsBooks. Both of them return list of Book type, but you dont know the list before contacting BE.

But there is API on cache to solve this nicely by using cacheResolvers

SachaG commented 6 years ago

Oh, I forgot about cacheResolvers but I think it's exactly what @PolGuixe needs.

rosskevin commented 6 years ago

I'm finding this talk about the cache helpful - https://youtu.be/zWhVAN4Tg6M?t=13m19s

rosskevin commented 5 years ago

Closing - housekeeping