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

feat(react): useFragment hook (BETA) #3570

Closed
wants to merge 25 commits into from
Closed

Conversation

JoviDeCroock
Copy link
Collaborator

@JoviDeCroock JoviDeCroock commented Apr 22, 2024

Resolves #1408

Summary

This introduces a new hook to the React package that has two main purposes. On one hand it will mask over the data it gets from its parent component given a fragment, so in the following scenario:

query GetTodo { todo { ...TodoFields completed } }
fragment TodoFields on Todo { id text }

When we'd pass the resulting data from GetTodo.todo into a component and leverage useFragment we wouldn't be able to get to the completed parameter.

The second goal is to establish a loading paradigm for deferred queries, this means that given the following document

query {
  todo {
    id
    text
    author { ...AuthorFields @defer }
  }
}

fragment AuthorFields on Author {
  id
  name
}

When the initial payload comes in we can display the id and text, during that we display this query as stale however we can make this hook in deeper with React paradigms by leveraging suspense when the deferred boundary isn't complete and suspense is enabled we can suspend. Even without suspense this brings the benefit of not having to check stale and being able to render independently of the result.stale flag.

Notes

The heuristic used to assess whether we are using is the presence of undefined basically data can only be undefined when it has not been queried or while it's being streamed in.

I opted for a slightly different approach to useQuery, in useQuery we are subscribed to the request and when the resolved query comes in we call resolve on the promise we threw to React. In this one I opted to purely base it on props, I tested this in this demo.

I opted to create the request with our code we use for operations but replaced variables with data to ensure every entity gets its own cache entry.

A lacking piece at the moment would be directives, if a user leverages @include() with a variable then we'd need access to the parent variables to uphold our heuristic of when to set this to loading. To counter-act this we check whether the field is undefined and when it had one of these directives we accept the absence of the data.

In the future we'll need to find a way to resolve() the promise, this is currently impossible as we have no way of establishing a stable identifier across concurrent invocations of a fragment given unique sets of data.

Todo

  • support gql.tada FragmentOf dance for data
    • Decision: unwrap with readFragment before using useFragment
  • verify whether TypedDocumentNode types work correctly
  • handle nested fragments in the FragmentDefinitionNode

Copy link

changeset-bot bot commented Apr 22, 2024

🦋 Changeset detected

Latest commit: 11727b6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
urql Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

): { data: Data; fulfilled: boolean } => {
const maskedData = {};
let isDataComplete = true;
selectionSet.selections.forEach(selection => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think, this would need (named) fragment spread support in before a merge. It's simply too uncommon not to compose fragments, and that requires named fragments. There's probably something to be said though about that being optimised in the future

(Note: we talked about nested signals/reactive proxies, so that could be an approach to feed information on changes in above maskFragment and to hand info to the next nested maskFragment for a different document; not in scope for the PR anyway)

Question is what full-masking (like in this implementation) would also look like given heuristic/non-heuristic fragment matching.

Last note is that we could align this a bit more to the Graphcache implementation here so it vaguely matches. Just in the overall style, so we have a bit of parity in case we decide to share code. That's optional though and not necessarily actionable, unless this gives you an idea of course

Copy link
Collaborator Author

@JoviDeCroock JoviDeCroock Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few things bothering me about doing that, we should def do FragmentSpread as alluded to in the todo, however what bothers me is that these spreads could be suspended themselves. I wanted to create the assumption that a FragmentSpread with @defer wouldn't be registered in useFragment but there would be an expectation that this is passed on to a new component.

For InlineFragment I have implemented it similar to Field and defer would trigger internal to the component.

In writing this I realised that we actually can't do include/skip as it stands because we don't have access to the variables the parent-operation was executed with, this could be a limitation for now but weakens the heuristic we use to see whether something is deferred significantly.

RE the graphCache point, I can add heuristic fragment matching to this if that's desired, for now this only works on concrete matches.

EDIT: added an initial heuristic for include/skip in 6c7eb36 and naive heuristic matching in 8ccfba1

@JoviDeCroock JoviDeCroock marked this pull request as ready for review April 23, 2024 12:39
@JoviDeCroock JoviDeCroock requested a review from kitten April 23, 2024 12:53
if (newResult.fulfilled) {
return { data: newResult.data, fetching: false };
} else if (suspense) {
const promise = new Promise(() => {});
Copy link
Collaborator Author

@JoviDeCroock JoviDeCroock Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being able to resolve this Promise might be important for streamed-SSR, I am not sure 😅 the issue here being that we can't keep a stable reference to this cache-entry as we face two challenges

  • This could be a series of useFragment, think of a list of todos. This means we'd be able to create a key for the document, however we'd then share this key across all siblings which could be streamed in out of order.
  • When we add data to the key this means that when the previous entry is completed due to the missing part streaming in that we update the key to a new state. This becomes problematic as we never actually resolve the promise, storing the previous reference in a ref isn't possible as React tears down the previous state.
  • Leveraging useId is also not possible as that isn't stable across Suspense invocations

One solution I can see is deriving a data that follows along the document and purposefully omits any include/skip and defer from the data and uses that to derive the key. This would essentially be similar to creating a __typename:id as they key, which could also be relevant here.

EDIT: suggestion implemented in ea665f3

@JoviDeCroock
Copy link
Collaborator Author

This proposal has run into kind of a dead-end after trying it out with streamed rendering, the issue resides in the fact that during SSR React does not do any re-rendering which means useFragment will never be re-evaluated.

For this to work we'd have to establish a subscription in our urql-client where this fragment is used, this subscription would need to bring the data to the useFragment, this adds additional complexity to this API proposal because we'd need to be aware on which entity we are listening for changes and hook into the correct query. to evaluate this fragment against.

@JoviDeCroock JoviDeCroock deleted the use-fragment-mvp branch April 24, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Fragment Suspense boundaries in React bindings
2 participants