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

Clarify expected behavior of fetching and stale #2282

Closed
ianschmitz opened this issue Feb 16, 2022 · 15 comments
Closed

Clarify expected behavior of fetching and stale #2282

ianschmitz opened this issue Feb 16, 2022 · 15 comments
Labels
documentation 📖 This needs to be documented but won't need code changes

Comments

@ianschmitz
Copy link

Hi! 👋

This is more of a request than a bug report, but perhaps it's a bug?

I'm a new user of urql. I'm evaluating the library coming from a fair bit of exp. with react-query and graphql-codegen. One of the issues i ran into was the behavior of fetching and stale.

I have a query that looks something like the contrived example below:

query Todo($id) {
  commonStuffs {
    id
  }
  todo(id: $id) {
    id
  }
}

Where commonStuffs is the same result no matter which todo is being queried. Now where i stumbled was the following:

  1. browse to todos/1 which runs above query with id = 1. fetching = true and stale = true
  2. browse to todos/2 which runs above query with id = 2. fetching = false and stale = true

In the second todo query, part of the query was already available in the cache and thus we got "partial results" (i think that's how you refer to them?). I was a bit tripped up by the nomenclature of the two states and it felt awkward.

For example i might have some conditional logic like so:

if (!fetching && !data?.todo) {
  return <NotFound />;
}

I changed it to the following to avoid a flash of "not found" before the second query finished, but it felt quite awkward:

if (!fetching && !stale && !data?.todo) {
  return <NotFound />;
}

One of the nice things about react-query was the type discrimination when checking isLoading. When you had a check that guaranteed that isLoading = true in my above example, then you knew data would be defined, even if data.todo was null.

if (!isLoading && !data?.todo) {
  return <NotFound />;
}

// `data` is defined here
return <div>{data.todo?.id}</div>

I'm not sure what the outcome of this issue should be. IMO the second query should have said it was fetching - because IMO it is fetching data? Perhaps we need some documentation around these states, and suggestions for how to deal with fetching/stale states, best practices, etc.

urql version & exchanges:
urql@2.1.3
@urql/exchange-graphcache@4.3.6

@ianschmitz ianschmitz added the bug 🐛 Oh no! A bug or unintented behaviour. label Feb 16, 2022
@kitten kitten removed the bug 🐛 Oh no! A bug or unintented behaviour. label Feb 16, 2022
@kitten
Copy link
Member

kitten commented Feb 16, 2022

Yep, that's totally expected behaviour, but with the added difficulty that without knowing how you configured the query, what request policy you're using, how you've set up Graphcache, and whether you're using schema awareness, the exact explanation will differ slightly 😅

The general explanation that always applies is pretty simple. There's a varying level of operations going on, of which there are two. These are also associated with varying levels of certainty.

The first important thing to note is that it may be interesting to note that stale is a flag on @urql/core's OperationResult. This means, regardless of the bindings you're using, "staleness" is a global concept that applies to any given result.

This is compared to fetching, which is noticeably not expressed as flag — afterall, without a result being present yet, there must be a state where we're waiting for a result.
The crucial hint here is that fetching is purely local state.

In other words, as long as your local query, in a component, is waiting for a result, it will indicate that it's doing so via fetching. This may or may not be state that's safe to be displayed, but that's up to the particular usecase. In many cases, you may find that it's extremely safe to display, in others, you may want to wait for stale to flip to false.
Either way, both flags are guaranteed to eventually settle and flip to false, and both flags may start out as true or be true while a result is already available.

In the UI state we may actually be waiting for a different result (maybe because the query and/or variables changed) which means that fetching indicates that a new result is coming

However, stale indicates that a newer result will be available soon, but for the same result.

This is important because in Graphcache there are many mechanisms that can cause a result to update in the background, which often triggers network requests.

With both flags there's some "magic" going on behind the scenes that increases their certainty.
For instance, a query may already be rendered elsewhere in the app, which means a result may be available immediately regardless of which cache you're using. Generally this is related to us differentiating between an initial state and a subscribed state. Whenever bindings start a subscription the cache synchronously see this request, which gives the cache an opportunity to give an initial state to us immediately, even if cache-and-network is used for instance.

In this sense, we're basically anticipating GraphQL becoming more equipped to deal with partial and stale states itself. Nullability operators can help us here for instance: #2018

On the other hand, we're also taking React Suspense and Relay as a future inspiration. We don't see "staleness" as a topic that's completely finished for us. Rather, we can see component-level fragments and utilities helping us to say what's required, can be stale, or should never be stale on a component-level.

We completely understand that this may sound more compelling for React users than other frameworks, but our greater ambition here is to eventually find time to implement Relay-style composition patterns, which eventually mean that we'll make "harder recommendations" and patterns around this.

But for now what this means is: in some cases you'll have to be aware that stale: true provides insight into the level of uncertainty around loading and you'll have to decide when a UI will realistically have to communicate to a user whether data may change.

To close this off, there are many examples and comparisons to this. Of course, all our bindings attempt to store their previous state while they're fetching but with normalised caching and cross-app refetching this becomes more complex;
If we know a screen is changing due to a mutation that's happening elsewhere in the UI would we want to indicate this to a user?

This obviously is a more pressing issue when you talk about null fields in "simple" getter queries for details views. There it makes sense to respect stale on a page level even if an item is initially null and show a loading state for just a bit longer.
(Afterall, we don't want to show a 404 state and then flip to content, but at the same time, what's the more likely state? Content or no content?)

Nested data is also a space where the @defer directive is going to become more interesting as it allows for granular loading states without having to split up queries — hence playing into cases that will then "supersede" the stale flag and will play into this fragment/component level state more.

Hope that makes this a little clearer ✌️
It's obviously a lot of notes, and I'll have to look into where we can highlight stale a little more clearly. But practically speaking, it's intended to be intuitive currently. It should be obvious (hopefully) when respecting it is necessary, but that clearly necessitates that people are aware of it 😄

@kitten kitten added the documentation 📖 This needs to be documented but won't need code changes label Feb 16, 2022
@ianschmitz
Copy link
Author

You guys are awesome. Thanks for taking the time to detail this. I think some of this could be great in the documentation.

Some clarifying questions:

This is compared to fetching, which is noticeably not expressed as flag — afterall, without a result being present yet, there must be a state where we're waiting for a result. The crucial hint here is that fetching is purely local state.

Do you mind elaborating on what you define as a flag/local state? I'm not quite following the difference here. As a naive consumer, fetching and stale are both "flags" in my eyes. Not trying to be facetious, just trying to understand the nuance.

In other words, as long as your local query, in a component, is waiting for a result, it will indicate that it's doing so via fetching.

In my example above, todos/2 was waiting for a result, the todo item with id 2.

However, stale indicates that a newer result will be available soon, but for the same result.

In my example above with the second todo, it was a different result. Should i have expected fetching = true, stale = false then? I'm not sure what they should be set to, since part of the query result is common and doesn't change and is already in cache, but the other half (the todo) was a new query and hadn't been fetched before.

This is important because in Graphcache there are many mechanisms that can cause a result to update in the background, which often triggers network requests.

I imagine an example is something like cache.invalidate? Or perhaps a mutation updating partial state and then graphcache fetches the rest so it's consistent?

Again, thanks for taking the time. Much appreciated!

@kitten
Copy link
Member

kitten commented Feb 16, 2022

Do you mind elaborating on what you define as a flag/local state? I'm not quite following the difference here. As a naive consumer, fetching and stale are both "flags" in my eyes

Yea, no worries! Let me explain it conceptually, and then link some code examples.
We basically have streams, like explained on the Architecture page. fetching is something that consumers of these streams can determine. Before the stream returns a result fetching: true applies, when we have a result then we flip to fetching: false. This is basically UI state because urql's Client doesn't tell us actively that fetching applies, but instead the "process" of waiting for the result from the Client itself is this state.

It can probably be seen clearest in the Vue bindings:

Meanwhile, stale is part of the result. To give some examples:

I'm not sure what they should be set to, since part of the query result is common and doesn't change and is already in cache, but the other half (the todo) was a new query and hadn't been fetched before.

Usually, that's not a problem. I'm obviously not sure what your UI looks like, but I suspect you'd be displaying both pieces side-by-side. So, if you get stale: true and one of them is null, they may still be loading. But it's likely safe for them to be displayed when you do have non-null results but stale: true applies.

I imagine an example is something like cache.invalidate?

Precisely! When information "goes missing" it's pretty likely that Graphcache will trigger more requests, either for the "current" query that it receives or other ones (i.e. when something is invalidated and hence re-queried from the cache)

@ianschmitz
Copy link
Author

Feel free to close if you'd like.

I think the main outcome i'd love to see is some docs improvements. I'm still trying to wrap my head around everything you've said.

Thanks so much!

@erquhart
Copy link

erquhart commented May 5, 2022

I think I'm dealing with this same issue and I'm quite at a loss. I have:

  • A React Native project with two screens, one clicks through to the next
  • They both use a literally identical query, like copy paste identical
  • I've tried swapping them, the components don't matter, it's which one comes first that matters
  • I've tried copy/pasting the entire component so the whole screen is identical, makes no difference
  • Calling executeQuery (network-only) behaves differently on these two screens
  • On screen 1:
    • triggers the request
    • sets fetching: true until complete
    • sets fetching: false
    • stale remains false throughout
  • On screen 2:
    • triggers the request
    • sets stale: true on Screen 1 (because both screens are mounted at this point)
    • sets stale: true on Screen 2
    • sets stale: false for both screens
    • fetching remains false throughout for both screens

I can walk through this process, navigate back to screen 1, and keep doing it over, and these two screens behave precisely the same every time, making me suspect this is by design.

I've figured out that I can use false || stale to catch the signal I'm looking for (setting the value of the refreshing prop for RefreshControl), as you stated that we can expect both to settle back to false reliably, but the difference in behavior between the screens has me lost.

I'm not blocked so I'm not asking anyone to debug, just curious if this behavior is expected and would love to understand why.

cc/ @kitten since you had such a firm grasp in earlier comments

@Coobaha
Copy link

Coobaha commented May 26, 2022

@kitten @JoviDeCroock What will be the recommended way of re-executing background queries without marking existing results as stale? My use case is custom refocus exchange, it compares refetched data with existing and only updates if it was actually changed. This way it will only re-render if response did change. If existing data is equal to response data - it returns existing result and also sets response.data to cached one, so other exchanges will receive same object reference.

Main issue is that client.reexecuteOperation always marks data as stale, and this triggers re-renders cause result.stale is changed in react hooks

@JoviDeCroock
Copy link
Collaborator

This is kinda off-topic but why would a re-render be bad? It is the guarantee in React

@Coobaha
Copy link

Coobaha commented May 26, 2022

Well, from a view perspective there is nothing to update, and also there could be heavy queries to render, that actually didn't change but it will be re-rendered on focus. Will be happy to open another issue/discussion if this is off-topic, if needed.

If this stale: true here can be set to false - it will solve this issue, similar to staleWhileRevalidate flag in ssr exchange

https://github.com/FormidableLabs/urql/blob/8892ea815a4d9d32d2f550ae92516c7be5cacd1c/packages/core/src/client.ts#L340-L342

Upd:

Ended up with a dedicated client (without cache exchange) for doing background fetches and propagating only changed results back to main client results stream, which is probably the way how it should be done. But being able to re-execute query without marking results as stale would simplify this a lot.

@ceefour
Copy link

ceefour commented Jun 13, 2022

In my case I have a problem where stale is always true, triggering React's infinite re-render:

Screen Shot 2022-06-13 at 12 57 05

I'm still looking for other threads which is more relevant to my issue, but this is the first google result I found.

with requestPolicy undefined or network-only, it's still infinite re-renders.

However, with requestPolicy undefined, the stale is always false, which is weird.

Self note -> https://jsramblings.com/how-to-check-if-your-component-rerendered-and-why/

@kitten
Copy link
Member

kitten commented Jun 13, 2022

@ceefour That's really odd. Mind opening a new discussion thread for that? This usually happens in one of two cases:

  • You're using cache-and-network by default or at the query and you're using the query in two places
  • or; you're using the normalised cache and due to the config some piece of data is always missing (this shouldn't trigger an infinite rendering loop normally, but it's possible)

@ceefour
Copy link

ceefour commented Jun 13, 2022

Update: @kitten regarding my issue before #2282 (comment)

It was caused by useQuery({context}) option. The problem was I gave a literal object as context, and urql did not like it.

After I replaced context with a global/singleton object, the problem was resolved.

It's definitely not relevant to this issue, but https://formidable.com/open-source/urql/docs/api/urql/#usequery doc should have BIG warning on context argument. Because other parameters, like variables, requestPolicy etc. can be specified literally and useQuery still works fine. So the different behavior in context was very frustrating.

I almost fell back to using client.query() but I decided to retry without using context at all and that's when I noticed something wasn't right.

@nandorojo
Copy link
Contributor

@ceefour I think you just have to memoize the context.

@robertherber
Copy link
Contributor

I cannot see how it's possible to display a loading indicator in a reasonable way currently, what am I missing?

  • fetching can be false when refetching, so we can't rely on it for displaying a loading indicator
  • stale can both indicate a background update happening - but also just that the cache is out of date

The closest I can see currently is checking if either fetching or stale is true - but we'll get a false positive if we just have an outdated cache (I'm working with an offline-enabled app currently - so that would be expected to happen).

Am I missing something here?

@bolandrm
Copy link

bolandrm commented Oct 25, 2022

+1 This seems unusable at the moment.

I tried upgrading from URQL 1.11.6 to 3.0.3 this weekend.

In 1.11.6, fetching changes to true any time a request is being made. So when I would run executeQuery to refetch the data for a screen, I would see fetching change to true.

This doesn't seem to be the case in 3.0.3 (fetching states false throughout the refetch) and would make many things more difficult. For example, it's unclear how I would implement drag to refetch without manually keeping track of the state of the refetch.

@kitten
Copy link
Member

kitten commented Mar 19, 2023

I believe #3079 (and #2962) in upcoming versions will help with this.

We've struggled to write documentation that is instructive and in the rigth order, and hence, sometimes the documentation skips over some behavioural details or some minor API details.

We belive that TSDocs can solve this much more extensively, and we'll be replacing our API docs with them.

The reason is that TSDocs have the ability to show up in all editors with a TypeScript LSP integration, something that VSCode comes with natively, and something many TypeScript developers set up for a good experience.

This allows us to communicate small or big API functionality where it matters.

Specifically, fetching and stale now have many more notes. In each binding it's even called out that fetching is a "local UI" state, so to speak, and it points out when stale becomes true.

@kitten kitten closed this as completed Mar 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📖 This needs to be documented but won't need code changes
Projects
None yet
Development

No branches or pull requests

9 participants