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

(graphcache) - result.fetching is always false in some cases when using requestPolicy cache-and-network #2002

Closed
carloslibardo opened this issue Sep 30, 2021 · 9 comments
Labels
bug 🐛 Oh no! A bug or unintented behaviour.

Comments

@carloslibardo
Copy link

Hi,

Awesome work!

I am trying to use urql cache in my application but i'm having an issue with that =/

Isn't in all queries, but in some places this happen, i'm doing the query, but the fetching prop is always false.

image

loading is just an alias to fetching.

If i changed requestPolicy to network-only, it's work, for some reason, i think that the component understand that already read a value from cache, then don't show loading, but i'm not sure.

I have some idea about that, but still don't know how solve that.

I have two queries, first query get all users to populate a user table, the second query is triggered when i click in a table row, selecting a specific user.

Then i have something like this:

query 1:

{
    organization(id: "test") {
        users {
            id
            name
            birthDay
        }
}

query 2:

{
    organization(id: "test") {
        user(id: "123") {
            id
            cellphone (focus that)
        }
}

I'm not showing loading at component that trigger query 2. I don't know how solve that, but i think that when i get a user with id 123, and in the last query a i found a array with users, and have some object of user, with id 123, the cache undersand that is the same value.

urql version & exchanges:

"urql": "^2.0.5",
"@urql/exchange-auth": "^0.1.3",
"@urql/exchange-graphcache": "^4.3.5",
"@urql/devtools": "^2.0.3",
"@urql/exchange-multipart-fetch": "^0.1.13",

Steps to reproduce

I don't know how, maybe i should make a demo if need.

Expected behavior

Fetching first setted to true in second component (running query 2)

Actual behavior

Fetching first setted to false always in second component (running query 2)

Someone have any suggestion? I think it's something silly that i have forgotten.

Thanks for your help =)

@carloslibardo carloslibardo added the bug 🐛 Oh no! A bug or unintented behaviour. label Sep 30, 2021
@JoviDeCroock
Copy link
Collaborator

Have you checked the result.stale prop? When a stale result is served while we do a background fetch that is set to true

@carloslibardo
Copy link
Author

Hello @JoviDeCroock, there is a print about loading and stale prop:

image

Query result with session came after stale setted to false, before that, the query result is:

{
    "organization": {
        "id": "test",
        "session": null
    }
}

@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Oct 10, 2021

So that seems correct right 😅 we are serving a result from cache (partial result) and are fetching in the background which we can derive because of result.stale being true there. I'll close this issue

@carloslibardo
Copy link
Author

Wait a moment, why it's getting result from cache? This doesn't make sense, i doesn't have this data from cache as you can see in query 1, i'm getting users, and the unique "cache" data, should be id.

I think your affirmation is right, we are serving a result from cache (partial result, in this case, id), but is not the correct behavior.

In this way, i can't show loading for the front and doesn't have any data available to show in front.

@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Oct 12, 2021

Your schema tells that the missing fields are nullable so it's rightfully reported as partial in the future #2018 might solve that

@Dremora
Copy link
Contributor

Dremora commented Nov 10, 2021

Speaking as someone coming from the apollo-client, this behaviour is surprising. I have a field marked as nullable, which we base business logic upon in the frontend. The logic (in this case, redirect to a 404 page in case of a missing field) runs before we get a chance to fetch the missing data.

There is a semantic difference between a (as of yet) missing value and null. In case of a non-nullable value, we might be returning out-of-date data, but at least it's guaranteed that the field had that value at some point in time. In case of a nullable field, the field might never have been set to null.

The workaround for me is to simply not pass the schema property to the cacheExchange, disabling schema awareness.

@kitten
Copy link
Member

kitten commented Nov 11, 2021

@Dremora Not quite sure I get the context of the missing fields in your case; I'm assuming the redirect to 404 is a custom implementation. It very much depends however how you implemented that in your GraphQL schema. A null value is a funny thing in GraphQL because there are multiple ways to interpret it depending on how your schema implicitly returns it. It's quite common to treat Query.item for instance as "missing" and default to null rather than issuing an error or special object, which may not be true for other parts of the schema.

That specific semantic difference will definitely depend on how you're building the schema. This actually also recently came up here: #1365 (reply in thread)
(Feel free to ask more questions in a discussion of your own if you'd like to fan out on this)

To summarise that thread, partial data in Graphcache is explicitly a semantic layer on top of existing schema data. But in the absence of @defer and nullability operators as a feature, it isn't possible to choose a different intuitive semantic as an alternative to the schema itself (yet). So the good news is that a solution is coming to GraphQL, the bad is that it isn't here yet (unless your API supports @defer since the protocol is already implemented in urql and Graphcache)

However, you can always choose to defer "destructive logic" like a redirect by not applying it when a result is marked with stale: true. This flag is independent of schema awareness, and may even be relevant without it. All it indicates is that more data is being fetched in the background. If it's set (and it may also be set because of cache-and-network) it's safer to wait for the API's next result rather than doing an action like redirecting immediately ✌️

@Dremora
Copy link
Contributor

Dremora commented Dec 9, 2021

@kitten apologies for late reply. Yes, 404 is indeed a custom implementation. It looks like this:

  useEffect(() => {
    if (data && !data.item) {
      void redirect('/404');
    }
  }, [data, data?.item, redirect]);

I rely on the item property being nullable to determine whether an entity that we request (with its ID taken from the URL) exists. So the semantics expected here: if the property is null, it's not that we don't yet have the knowledge about the value of this field, but that we do have the knowledge, and its value is null.

When it comes to @defer, it seems to be solving the opposite problem - reusing partial results from the cache with non-nullable fields (and temporarily returning them as null). So, is it correct to assume that once @defer is here, urql will change its semantics (and the current behaviour will be opt-in on per-field basis via @defer)?

@kitten
Copy link
Member

kitten commented Dec 9, 2021

@Dremora Well, nullability opeators is actually what you likely want together with schema-awareness. It'd allow you to explicitly say that the frontend does not handle a certain case, in other words:

query ($id: ID!) {
  item!(id: $id) {
    id
    ...Item
  }
}

Here we say that item! is required by annotating it with !, which means that we say "this should never be null". In the future, the server-side will then be able to instead error out when null is returned, which means you'll get either data: null or data: { item: null } to react to, and Graphcache knows that defaulting to null will be unacceptable, no matter what the schema says about it.

In reverse item? would allow you to do the opposite, by marking a field as always optional, which means that Graphcache is then able to optimistically null this field out if it otherwise has enough data to fulfil your request.

The spec for this however is taking quite a while, so I'm currently considering to support a Relay-like approach to this by adding @required. This would be quite a huge change, but in essence, the idea is that it may be able to support the same idea by providing a Graphcache-only primitive to treat the field as required https://relay.dev/docs/next/guides/required-directive/

However, in either case, if you're doing a redirect only when an entity is missing, it's often sufficient to wait for data to settle, i.e. to change your check to:

  useEffect(() => {
    if (!result.fetching && !result.stale && !result.data?.item) {
      return redirect('/404');
    }
  }, [result]);

In general, checking for !!data is something I'd be careful about, because data can also be null in the presence of field errors, or missing in the presence of request errors. So, !result.fetching is always a definitive answer on whether you should wait in general. The !result.stale flag is basically to circumvent the "optimistic" i.e. client-side partial result if you're using schema awareness.
In certain cases it's desireable not to react to data that is changing. The result.stale flag basicallt says "you can expect a new result here soon", so if you're checking for !result.stale you're explicitly saying that you don't want a result that's partial or changing soon.

This also has other uses. Suppose you're on a list page and because you redirect the user to a "create item" page. Let's say you don't have Graphcache updaters set up, and hence either use the requestPolicyExchange (refetch on a TTL basis) or switch the listing page to cache-and-network.
You now redirect from the "create item" page back to the list page, because the user has created the item. However, your cache is still refetching and gives you a stale result, which is the empty list, so you redirect them again to the "create item" page. Situations like that are avoided by ensuring that you check result.stale before you do any "destructive" action like redirecting, since it informs you of pending background fetches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Oh no! A bug or unintented behaviour.
Projects
None yet
Development

No branches or pull requests

4 participants