-
-
Notifications
You must be signed in to change notification settings - Fork 453
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
Conversation
🦋 Changeset detectedLatest commit: 11727b6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
686f300
to
17f8336
Compare
if (newResult.fulfilled) { | ||
return { data: newResult.data, fetching: false }; | ||
} else if (suspense) { | ||
const promise = new Promise(() => {}); |
There was a problem hiding this comment.
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 thekey
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
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 For this to work we'd have to establish a |
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:
When we'd pass the resulting data from
GetTodo.todo
into a component and leverageuseFragment
we wouldn't be able to get to thecompleted
parameter.The second goal is to establish a
loading
paradigm fordeferred
queries, this means that given the following documentWhen the initial payload comes in we can display the
id
andtext
, during that we display this query asstale
however we can make this hook in deeper with React paradigms by leveragingsuspense
when the deferred boundary isn't complete and suspense is enabled we can suspend. Even withoutsuspense
this brings the benefit of not having to checkstale
and being able to render independently of theresult.stale
flag.Notes
The heuristic used to assess whether we are using is the presence of
undefined
basicallydata
can only beundefined
when it has not been queried or while it's being streamed in.I opted for a slightly different approach to
useQuery
, inuseQuery
we are subscribed to therequest
and when the resolved query comes in we callresolve
on the promise we threw to React. In this one I opted to purely base it onprops
, I tested this in this demo.I opted to create the request with our code we use for operations but replaced
variables
withdata
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 toloading
. To counter-act this we check whether the field isundefined
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
gql.tada
FragmentOf
dance fordata
readFragment
before usinguseFragment
TypedDocumentNode
types work correctlyFragmentDefinitionNode