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

RFC: Fragment Suspense boundaries in React bindings #1408

Open
kitten opened this issue Feb 22, 2021 · 10 comments
Open

RFC: Fragment Suspense boundaries in React bindings #1408

kitten opened this issue Feb 22, 2021 · 10 comments
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release

Comments

@kitten
Copy link
Member

kitten commented Feb 22, 2021

Summary

Overall, our new Suspense implementation is working a lot better, as expected, after getting some feedback on it from the React team. This gives us the room to experiment with more uses of Suspense in our React bindings.

Today when we load a query with Graphcache we may run into cases where Schema Awareness kicks in, when it's enabled.

d13d7d912b7dd35e82cb6c98b36d9c65-2000-1

Today this already allows people to have a page-wide query that loads some data and displays partial data when the cache doesn't have enough information to fulfil the query in its entirety but so far that it can leave out optional fields.

This poses a challenge for implementations, where some sub-components of a page may have to switch between a partial and a non-partial state. Assuming that these have types in a TypeScript environment, dealing with partial data on its own isn't too difficult. However, implementing many cascading loading states can be a difficult task and Suspense can help here.

Proposed Solution

It should be possible for a component to check data for completion using a useFragment suspense hook. This hook accepts a fragment and a piece of data and suspends when the fragment isn't fully fulfiled and partial.

TBD: Exact API details pending #1221

Passive Update solution:

One solution that may work is to passively ingest React updates and Client results; i.e. the useFragment hook receives a fragment and data but will be used with a useQuery hook that also triggers suspense updates, hence we'll eventually pre-render up until the useFragment hook(s). This may either have a separate suspense boundary or use a suspense boundary above useQuery. The important detail here is whether it has its own boundary.

When a useFragment hook has its own boundary it'll likely have to look at all active queries and filter them down to ones that also consist the same fragment (by name). Hence, a code generation tool would be of prefereable use here. Once it detects that the same fragment occurs in an OperationResult it should update and either throw a suspense promise or return a result as needed. The result should be masked as per the fragment.

Requirements

  • A useFragment hook should accept data and a fragment and return masked data
  • The useFragment hook should suspend when its data is incomplete
@kitten kitten added the future 🔮 An enhancement or feature proposal that will be addressed after the next release label Feb 22, 2021
@n1ru4l
Copy link
Contributor

n1ru4l commented Apr 21, 2021

Will it be possible to use useFragment without suspense?

Having cache updates only trigger re-renders in components that consume the affected data via a fragment vs triggering a re-render via the useQuery is a pretty neat performance optimization in relay.

@kitten
Copy link
Member Author

kitten commented Apr 21, 2021

It isn't really what we were envisioning here to be completely honest.

GraphQL is pretty great because it fits the mental model of componentised apps really well. Ultimately it's a way of Modeling relational data as it flows down from top to bottom in an app. Fragments enhance this by defining data requirements at every step along the way, which is why they're so great when used at each component that passes on and splits up data.

What I've seen a lot is that in apps these responsibilities become easily conflated and these boundaries are ignored. But ultimately what I see work best is a differentiation between three types of components: "stateful components" (smart components), "structural components" (dumb components), and "presentational components" (often leaf components)

So often times, if a team was really careful about this, you'd have data requirements composed to some kind of "top level" component (a page, a modal, an async/split component) that executes the query; it then starts passing on that data to structural components.

A structural components only responsibility is to define data requirements and map those to presentational components and smart components. Therefore follows that any other component should not disturb this flow.

Stateful components can add on state and interactivity (or other data) along this flow and presentational components should mostly be concerned about UI and hence none of the other 'types' of components should be rendered by presentational components, if possible.

This fits really nicely into React's (and Vue's) Suspense vision. Consequentially Suspense is a mechanism for a structural component to render placeholders or loading elements when their data requirements aren't fulfilled.

How does that fit into this discussion?

It's possible for state containers to circumvent this structure. The simplest form of this and equivalent are providers in React; and often those are used for both data and state, which can lead to issues. So while a stateful container is as low in the tree as possible, libraries like "Valtio" and state containers are useful to keep stateful components low in the tree while still sharing updates and state changes across the app.

I have yet to see an app where "teleporting" data is a better choice than preventing excessive rerenders at a structural component boundary 😅 This is because as you get deeper into an app you exponentially encounter more work; however, interrupting this work and avoiding it at times can be cheaper than teleporting data.

Now, for these fragment containers to work a normalised cache needs deep integration with the bindings and client. And that's something that goes against our philosophy and principles that keep the API and internals simple and flexible.

But the other problem is that "teleporting data" can still be understood as a layered problem. Ultimately, to recognise the fragment of data it needs two pieces of information: A keyable entity and a GraphQL fragment.

So if you wanted this to also "teleport data" then you'd also need the keyable entity. So in other words, if we have useFragment we need to pass the fragment's data from the query in. Suspense is a nice fit here because we can suspend of that data doesn't fit our data requirements.

However, if we also think about "teleporting this data" and not use suspense then we may as well just send the entire fragment via context or something like a state container. Which is nice but not an interesting problem. All it would achieve is "skip" over some structural components which increases complexity at the top level component but only ultimately skips less than a handful of structural components.

So why suspense and this API?

Given that a preferable element structure passes GraphQL data on until it hits more and more presentational parts of our app this also coincides with loading boundaries and is exactly what Suspense will be good at; allowing React to prepare parts of the tree until all data requirements are fulfilled.

And Relay's mechanism isn't that different at that; it gets a handle for the fragment and checks for updates to enforce that the input data is actually updated when it needs to. My theory is that that's actually not necessary unless the suspense boundary that's triggered is above the query. But we likely can introduce a pretty simple mechanism to track what the fragment is accessing by exploiting that the fragment will be "seen" in the Client and the query sent as well.

Anyway, sorry for the long write up. But I wanted to clarify how this all fits together and how this actually closely aligns with what the React team intended Suspense to be used for and what Relay wants these patterns to look like ✌️ because when I see "performance optimisation" in this context, I believe it's easy to lose track of how things are actually used ergonomically by devs 😅💙

@n1ru4l
Copy link
Contributor

n1ru4l commented Apr 22, 2021

An easy example for a frequent app update would be updates of x/y coordinates of a specific element deep down in the tree. Triggering a re-render of the whole component tree just for updating the position is perseived as an overkill to me. If the coordinates are instead injected via fragment only the one component that consumes the data deep in the tree must be updated. The same also could apply for table rows and columns, a chat feed etc. There are many use-cases where teleporting can be handy.

@benlammers
Copy link

@kitten Any updates here? I have the same query running on multiple pages with different subsets of fragments. The issue is that when I navigate pages the second page's query does not suspend but provides the components with partial results from the cache while it queries the rest. This results in errors as data that should not be null is not available.

This poses a challenge for implementations, where some sub-components of a page may have to switch between a partial and a non-partial state.

Wondering if there is something I am missing. Should I be able to make a query suspend until it has fully fulfilled all its fragments?

@kitten
Copy link
Member Author

kitten commented Mar 15, 2023

when I navigate pages the second page's query does not suspend but provides the components with partial results from the cache while it queries the rest.

To be frank, that sounds more like you're relying on Graphcache's Schema Awareness mode?
So that's intended behaviour.

This results in errors as data that should not be null is not available.

That shouldn't ever be the case tbh 😅

However, the solution here is that we're waiting for GraphQL's nullability operators, to be exact. We're still playing around with alternatives (i.e. directives, which is Relay's approach) for those, but the intended behaviour is that you should be able to selectively specify which data a page/component needs and which it can lack while loading/displaying.

Re. fragmented loading: that's a more tricky problem. In general, even with Schema Awareness or the above, you have to make a decision. You could say that some components should display a loading state connected to stale: true, then Schema Awareness can be used in combination with partial loading states.

In the future, we'd obviously like to have something like useFragment and a corresponding fragment helper.

One of the requirements is that this must however work with all bindings, i.e. it wouldn't be React-specific, although React/Vue/etc may have a useFragment wrapper around such a primitive. Furthermore, the primitive must be able to differentiate between partial data that causes loading states, or partial data that doesn't cause loading states, which is still a pretty unexplored UX problem on our end.

However, that said, currently some of this is blocked because we're looking to provide TypeScript type-gen tooling of our own first, to enforce certain framework patterns that will make this easier.

@benlammers
Copy link

Thanks for the quick reply and insight!

This results in errors as data that should not be null is not available.

I spoke too soon here, looks like this is an issue in my own implementation with some assertions on nullable data.

Furthermore, the primitive must be able to differentiate between partial data that causes loading states, or partial data that doesn't cause loading states, which is still a pretty unexplored UX problem on our end.

This is what I would be looking for, looking forward to see the progress here, thanks!

@RIP21
Copy link

RIP21 commented Mar 15, 2023

However, that said, currently some of this is blocked because we're looking to provide TypeScript type-gen tooling of our own first, to enforce certain framework patterns that will make this easier.

Do you guys are using graphql-codegen under the hood I hope? :D I just got used to the tool so much don't want to learn another :D But I guess it's not totally suitable for that... but unsure.

Other than that, cannot wait for this feature to land one day. There are too many people that are lacking understanding of how you should build a query (e.g. from the bottom of the tree upwards, not from top to bottom). What are "component data requirements", and how do fragments play a role here. Having useFragment being a part of the library finally and even some generators on top of that will make these concepts much more obvious to a wider audience I hope.

@alexojegu
Copy link

Do you guys are using graphql-codegen under the hood I hope? :D I just got used to the tool so much don't want to learn another :D But I guess it's not totally suitable for that... but unsure.

I think they will use GraphQLSP but it is only a supposition.

@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Oct 28, 2023

I have been thinking an increasing amount about this issue lately and started thinking that maybe there is a middle ground RE maybe not needing Suspense but still encouraging a fragment co-location story.

Currently when a query receives an update, it does not matter how deep the updated selection is it will re-render entirely and everything until stopped by some kind of React/... specific bail method. We could alter this by optionally at first changing how these subscriptions are established. The heuristic that came to mind is per-selection so let's say we have the following document

query TodosView {
  todos(first: 10) {
    id
    text
    completed
    author {
      ...TodoFields_Author
    }
  }
}

fragment TodoFields_Author on Author {
  id
  name
}

we have a component where useQuery is invoked with this document and a child-component where we are executing useFragment with TodoFields_Author. We are able to subscribe the parent-component to Todo.<id|text|completed|author> and the child-component to Author.<id|name> now we can selectively propagate updates to one of these invocations.

Why does this matter?

Currently it feels a bit like a pitfall that we can have the following document with i.e. co-located fragments.

query ProductsView {
  category(id: $id) {
    id
    name
    filterOptions { id name values }
    products(take: $limit, cursor: $cursor) {
      id
      name
    }
  }
}

if we want to only be notified about products fetching we would have to split this up into two separate queries so we could still display the outer-boundary (I know there are ways around this but just saying 😅) which to me feels like a reason for folks to skip out of fragment co-location.

Concretely in urql this could mean having a first-class citizen on Client for fragment operations that subscribes a component to these fields, we would pass in the fragment with i.e. todo.author through props and have that be resolved through the cache which would do the appropriate subscription.

@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Dec 26, 2023

Building on the above, I see a few iterations here, we could in a minor release start supporting useFragment purely as a way to mask your data, in that case it doesn't yet have the characteristics of Suspense/... This encourages the fragment co-location story and the ideology of having components define their own data-requirements.

In a major release we could establish the separate subscriptions and work towards the idea of useQuery only watching fields (no fragment-spreads) and fragments re-rendering selectively (and managing their own Suspense and Error boundaries).

Note that nowadays we have three first-party clients that support Suspense (Vue, React and Preact w/ compat). I would say the major release still offers benefits to i.e. Preact and Svelte if we selectively re-render, in graphCache terms this would mean that the main query only creates dependencies for the non-spread entities and the fragments for their own contained fields on a given entity.

hbriese added a commit to zallo-labs/zallo that referenced this issue Feb 10, 2024
This is due to the whole token being passed as a prop (causing a re-render) not just the required fragment fields. This should hopefully be resolved with urql-graphql/urql#1408
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants