Multiple requests when using react suspense

danielkaxis commented 2 months ago

Describe the bug

The bug is that urql is somehow spamming multiple requests. Our application works like this basically. It has a set of modules and we iterate them returning lazy loaded components wrapped in React Suspense.

<Suspense>
  modules.map((m) => {
    component = lazy(m.component)
    return <Route 
      ...
    />
  })
</Suspense>

We don't know why this happens. But we suspect that it has something to do with how we structured one of our tree. We also suspect it has something to do with the lazy loading and react suspense In the example we have been able to reproduce the issue.

Consider this structure. It only happens when it is like this.

Query
  vehicles
Vehicles
  car
  cars
  bicycle
  bicycles

Reproduction

https://github.com/danielkaxis/urqlsuspensetest/tree/multiplerequests

__mocked_generated__ folder has the schema and urql resolvers.

yarn yarn serve localhost:8000 Open developer tools reload page

All logs show the query that has been called with console.count(). Car:name should be called 4 times but is called 14 times.

Why is it called so many times?

Comment out the mutation in App.tsx line 13.

await client.mutation(userMutation, {}).toPromise()

reload the page Car:name is now called only 8 times.

Why did that decrease the number of requests?

Urql version

"@graphql-tools/schema": "10.0.3",
"@urql/devtools": "2.0.3",
"@urql/exchange-execute": "2.2.2",
"@urql/exchange-graphcache": "7.0.1",
"graphql": "16.8.1",
"urql": "4.0.7",

Validations

[X] I can confirm that this is a bug report, and not a feature request, RFC, question, or discussion, for which GitHub Discussions should be used
[X] Read the docs.
[X] Follow our Code of Conduct

JoviDeCroock commented 2 months ago

Hey,

It's really unclear what you are expecting to happen here or how we would observe the issue. I would encourage you to give us clear reproduction/observation steps and upgrade your urql dependencies.

The logger API can also provide insight into what's happening https://commerce.nearform.com/open-source/urql/docs/api/graphcache/#cacheexchange

danielkaxis commented 2 months ago

Hey @JoviDeCroock,

Thanks for your reply. I have now updated the example and intructions. Thanks for the tip about the logger API. I will check that out.

ericclemmons commented 2 months ago

Hey @danielkaxis, thanks for opening this issue. I'll have to check into this as well because I just ran across this same problem when using React.lazy – useQuery underneath it doesn't seem to keep the Suspense boundary in a loading state 🤔

HitomiWin commented 2 months ago

Hey, I have tried to console log the issues with the logger function. It looks that queries once called are not cached ??? 🤔

urql1

urql

JoviDeCroock commented 2 months ago

As I've said before, it's quite hard to deduct what's going on in your reproduction. I'll try to spend some time on this over the next week, something that could go a long way would be for you to check whether it helps to

Hoist client creation to the global scope (as it stands you do initClient in a Suspense tree without memo/... which is dangerous in and of itself)
Identify at what version this started happening

danielkaxis commented 2 months ago

As I've said before, it's quite hard to deduct what's going on in your reproduction. I'll try to spend some time on this over the next week, something that could go a long way would be for you to check whether it helps to

Hoist client creation to the global scope (as it stands you do initClient in a Suspense tree without memo/... which is dangerous in and of itself)

Identify at what version this started happening

Sorry if my new example is also unclear. I really really tried :crying_cat_face: I'm not sure how I can explain it better. Thanks for the tip. I will try it.

JoviDeCroock commented 2 months ago

It is def weird that it's being re-queried, it almost feels like GraphCache is just removing data. This might just come down to layers and the mutation layer skewing here 😅 mainly because all operations are in-flight at the same time.

Currently don't have time to debug this but ye, might be something in there. Your thesis is correct though that this produces multiple requests, when using the debugExchange I can clearly see that every request is duplicated.

Update after spending the evening on this, the offending lines are situated here. Basically we should change this up to check whether it's not queued and whether the dispatched does not contain it yet like:

if (!queued && (!dispatched.has(operation.key) || operation.context.requestPolicy === 'network-only')) {
  queue.push(operation);
  Promise.resolve().then(dispatchOperation);
} else {
  dispatched.delete(operation.key);
  Promise.resolve().then(dispatchOperation);
}

That however re-introduces the issue we faced in https://github.com/urql-graphql/urql/issues/3254 where we do an optimistic mutation which results in the dispatch and when the response comes in the updater results in a second dispatch this second one does not go throug has the optimistic reexecute is seen as dispatched. Will need some more time on this but thought I'd update this issue already 😅

danielkaxis commented 2 months ago

It is def weird that it's being re-queried, it almost feels like GraphCache is just removing data. This might just come down to layers and the mutation layer skewing here 😅 mainly because all operations are in-flight at the same time.

Currently don't have time to debug this but ye, might be something in there. Your thesis is correct though that this produces multiple requests, when using the debugExchange I can clearly see that every request is duplicated.

Update after spending the evening on this, the offending lines are situated here. Basically we should change this up to check whether it's not queued and whether the dispatched does not contain it yet like:
if (!queued && (!dispatched.has(operation.key) || operation.context.requestPolicy === 'network-only')) {
  queue.push(operation);
  Promise.resolve().then(dispatchOperation);
} else {
  dispatched.delete(operation.key);
  Promise.resolve().then(dispatchOperation);
}
That however re-introduces the issue we faced in #3254 where we do an optimistic mutation which results in the dispatch and when the response comes in the updater results in a second dispatch this second one does not go throug has the optimistic reexecute is seen as dispatched. Will need some more time on this but thought I'd update this issue already 😅

Wow! This is amazing news. I can try to test this locally and see if the issue goes away :)

On a side note, hoisting of urql did not help.

danielkaxis commented 2 months ago

Thanks @JoviDeCroock! We will upgrade our packages and make sure it works. Much appreciated.

danielkaxis commented 2 months ago

Sadly, these changes did not make it for us. I have an updated example project with the upgraded packages.

"@graphql-tools/schema": "10.0.3",
"@urql/core": "5.0.2",
"@urql/devtools": "2.0.3",
"@urql/exchange-execute": "2.2.2",
"@urql/exchange-graphcache": "7.0.2",
"graphql": "16.8.1",
"urql": "4.0.7",

createClient is now used from @urql/core The updated example can be seen here. I have trimmed down the example even more to remove bloat code that is confusing. I have also added a query to an entity that exists directly under Query. The schema now looks like this in graphql.ts.

Query

User
Vehicle

Vehicle

cars
car(id)

Car

id
name

Compared to the example in the issue description Car:name is now called 10 times instead of 8 so there was a change with the update.

I will start from scratch. I have 4 cars. In Vehicles.tsx I have a query to fetch cars under Vehicles under Query. That query contains the id and name and that data should now be cached.

For every car I return a new component Car. In Car I have a query to fetch car with a specified id. That query should in theory give me the cached data. However it doesn't. It calls the resolver again.

This image shows only cars query in Vechicles.tsx. is removed and not querying any data. Car:name is called 4 times. That is good and what I expect.

This image shows together with car querying data for a specific id. For every car id I query Query:Vechicle:car(id). I expect that query to give me cached data. But it doesn't. It calls the resolver again. Car:name is now called 10 times.

As in the initial example I also remove the mutation before the lazy loading is complete. Car:name is now only called 8 times.

Interesting as well is that I also added a user query in Car.tsx but that query is returning cached data and only calls User query: 1 which is good and what is expected.

To be honest I don't think this is even related to anymore. When I remove Suspense and just render the components normally I get the same behaviour.

On a side note: In our real project we managed to workaround the issue of duplicate requests. We did that by using createClient from @urql/core instead of urql and downgrading @urql/core from 4.2.0 to 4.1.1. This did not work in my example though.

JoviDeCroock commented 2 months ago

I have been trying to make this work as good as I can but ultimately this is not really a structure we want to encourage, you can ofcourse make it work with an exchange that does the deduping for you.

The pattern here is a bit counter to GraphQL where you are creating a lot of requests throughout your application that are inter-dependent rather than one big selection-set.

danielkaxis commented 2 months ago

I have been trying to make this work as good as I can but ultimately this is not really a structure we want to encourage, you can ofcourse make it work with an exchange that does the deduping for you.

The pattern here is a bit counter to GraphQL where you are creating a lot of requests throughout your application that are inter-dependent rather than one big selection-set.

Alright, thanks @JoviDeCroock. I'm beginning to understand the issue. My ideas were pointing at that direction when I wrote the previous message. We do have several queries accessing the same entities like in my example. @HitomiWin tried the deduping you linked and said it seems to work.

The better way is to do one query. We will try this out. Thank you so very much!

danielkaxis commented 2 months ago

I want to post an updated example project for anyone interested in the outcome of this issue. https://github.com/danielkaxis/urqlsuspensetest/tree/multiplerequestsresolution

I removed the additional query document for fetching a single car. I now only have one document fetching all cars. The schema is still as before.

// Schema
Query
  vehicle

Vehicle
  car
  cars

Car
  name

// Query
export const VehiclesDocument = gql`
  query {
    vehicle {
      cars {
        id
        name
      }
    }
  }
`

To demonstrate: In Vehicles.tsx I query VehiclesDocument. https://github.com/danielkaxis/urqlsuspensetest/blob/multiplerequestsresolution/src/components/Vehicles.tsx

In Car.tsx I also query VehiclesDocument https://github.com/danielkaxis/urqlsuspensetest/blob/multiplerequestsresolution/src/components/Car.tsx

I have 4 cars I also see them called only 4 times :smile:

urql-graphql / urql