Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(react): useFragment hook (BETA) #3570
Changes from 7 commits
6e91676
be0b26e
65ae40e
0c722d9
8731db6
a318dfd
d9ef0cd
6c7eb36
88b84df
8ccfba1
dadf926
ec4fd17
ce0f551
b9a274a
1187faf
4aa9096
17f8336
d353496
892b8c7
57f1a58
fb6f98f
1e16372
ea665f3
85d7ed6
11727b6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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.useId
is also not possible as that isn't stable across Suspense invocationsOne solution I can see is deriving a
data
that follows along the document and purposefully omits anyinclude/skip
anddefer
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
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 nestedmaskFragment
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 aFragmentSpread
with@defer
wouldn't be registered inuseFragment
but there would be an expectation that this is passed on to a new component.For
InlineFragment
I have implemented it similar toField
anddefer
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