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

Implement minimal useFragment hook #8782

Merged
merged 13 commits into from
May 25, 2022
Merged

Implement minimal useFragment hook #8782

merged 13 commits into from
May 25, 2022

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Sep 13, 2021

Initial implementation of the idea I sketched in #8236 (comment) (though without previousData for now).

This API is still in development and is very much subject to change (hence the [WIP] tag). Watch this space for more details and examples in the coming days/week.

@trevor-scheer
Copy link
Member

I wanted this ~2 weeks ago, love to see it!

@hwillson hwillson added 2021-10 and removed 2021-09 labels Sep 14, 2021
@jayy-lmao
Copy link

jayy-lmao commented Sep 14, 2021

Super excited for this!

Part of a team of ~10 devs who are currently using my very-disfunctional useFragment 😅
Thanks for all your work on this, and looking forward to having a play around with it when it lands on the beta

@benjamn benjamn force-pushed the useFragment-hook branch 3 times, most recently from 1111b8e to 0a629c1 Compare October 4, 2021 22:41
@hwillson hwillson added 2021-11 and removed 2021-10 labels Nov 1, 2021
@benjamn benjamn force-pushed the useFragment-hook branch 2 times, most recently from 18addd5 to 020ad25 Compare November 16, 2021 19:39
@benjamn benjamn changed the base branch from release-3.5 to release-3.6 November 16, 2021 21:20
@benjamn
Copy link
Member Author

benjamn commented Feb 4, 2022

Since we are postponing useSyncExternalStore support until v3.7 (or whenever React 18 ships), I removed my commit that made useFragment leverage useSyncExternalStore.

A rebased-and-ready-to-cherry-pick version of that commit (35ccbd7) can still be found on this branch.

@Lalitha-Iyer
Copy link

Lalitha-Iyer commented Feb 28, 2022

Hi @benjamn ,
Stoked to see this work in progress.
Could you please expand on what is intentionally out of scope in the first version.
I am mostly interested in understanding, the steady state useFragment capabilities.
For example would we support paginated fragments.

// from: string | StoreObject | Reference;
// fragment: DocumentNode | TypedDocumentNode<TData, TVars>;
// fragmentName?: string;
// optimistic?: boolean;

Choose a reason for hiding this comment

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

what is optimistic used for.

Copy link
Member Author

Choose a reason for hiding this comment

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

InMemoryCache can temporarily have additional layers during mutations that use optimisticResponse, which then get discarded once the real data have been written to the root/non-optimistic layer of the cache.

When you're using useFragment, you can control whether it reads data from these optimistic layers (optimistic: true, the default), or just the root layer (optimistic: false). Reading from optimistic data is a good default because it's the same as reading non-optimistic data when there are no active optimistic updates, and reading optimistic data allows the optimisticResponse to be rendered immediately, and then the real response only needs to trigger a UI update if the optimisticResponse was wrong/incomplete.

| "id"
> {
from: StoreObject | Reference | string;
// Override this field to make it optional (default: true).
Copy link

@Lalitha-Iyer Lalitha-Iyer Feb 28, 2022

Choose a reason for hiding this comment

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

Why do we need to specify "from", since we already specify 'fragment`. I am guessing because we don't know which query it is bound to at runtime, and the 'id' of the type.

Copy link
Member Author

Choose a reason for hiding this comment

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

The options.from value represents the specific entity object (with some __typename that matches the fragment type condition, and some unique ID field(s)) against which the options.fragment should be applied/executed.

When you execute a query rather than a fragment, there's no mystery about which object to start with, since there's only one singleton ROOT_QUERY object. Fragments don't have that luxury, and always need to know the starting object. Occasionally it might be possible to guess the object based on the fragment's type condition; for example, fragment QueryFrag on Query {...} is always going to be a fragment that starts executing against the ROOT_QUERY object, but I'm not sure those special cases are worth exploiting, since they are not the common case.

The options.from argument is similar to the options.id field for cache.readFragment and cache.writeFragment, but a little more flexible (in case you don't know the string ID).

@benjamn
Copy link
Member Author

benjamn commented Mar 1, 2022

@Lalitha-Iyer Thanks for the review!

My two biggest open questions are:

  • How can we associate useFragment calls with the queries that support them, so they can share variables and display warnings when/if the query is removed or stops providing the data useFragment expects?
    • There's a possibility this belongs on the query side, as an option to useBackgroundQuery, but my current thinking is that useFragment can bear most of the burden.
  • Since GraphQL errors are not currently stored in InMemoryCache (a promising but tricky idea), and since useFragment is a direct binding to the InMemoryCache, how can consumers of useFragment find out about relevant errors that might impact the data they (don't) receive?
    • If useFragment had a reliable way to name the queries it depends on, we might also be able to expose those queries' latest errors in the useFragment result. This is appealing because it's not blocked on caching errors, and reuses the existing error logic in the client (outside of the cache).

When you asked about paginated fragments, were you thinking of something like putting a fetchMore function in the useFragment result object?

@Lalitha-Iyer
Copy link

Thanks for the clarifications @benjamn !

When you asked about paginated fragments, were you thinking of something like putting a fetchMore function in the useFragment result object?

I didn't mean to suggest any implementation, I was curious to know the breadth of capabilities we plan to make available long term. As a contrived example, one way to think abt the scope would be: what is the lifecycle of a fragment and what controls will be available to manage fragments ( accessing already cached data -> subscribing to changes -> refetching/pagination -> garbage collection).

With the lifecycle/capabilities listed, we could then try to model fragments either in the current way ( loosely coupled from queries ) or a stricter 1:1 mapping or a 1:many ( depending on whichever approach solves our usecases).

@Lalitha-Iyer
Copy link

Lalitha-Iyer commented Mar 4, 2022

@benjamn ignore my previous comment, I probably misunderstood fragments. Reading the Graphql spec if fragments are a reusable set of fields that can be applied to any query then your approach of not binding to a query makes senss. Your open questions around error handling and at runtime mapping fragment to one/more queries seem reasonable. Thanks again for taking time to clarify.

@benjamn benjamn modified the milestones: Release 3.6, Release 3.7 Apr 6, 2022
benjamn added 12 commits May 25, 2022 13:28
#8782 (comment)

Previously, the ListFragment was effectively ignored because its type
condition (on Query) did not match the "Item" __typename.

Although the code was definitely wrong before, this change ends up not
making a visible difference because the Item.id field provided by
ListFragment is the default field used for normalization, so it gets
picked up and used regardless of whether it's mentioned in the query
(even when ListFragment is completely ignored, as it was before).

To make this change matter in a visible way, I added an extra root query
field to ListFragment to strengthen the tests.
Although useFragment increases bundle sizes by this measurement, it also
provides an alternative to useQuery and useLazyQuery that should prove
smaller (if used alone, or with `useBackgroundQuery`) than using the
existing hooks.
@benjamn benjamn force-pushed the useFragment-hook branch from b2a2a1d to 1e37053 Compare May 25, 2022 17:46
@benjamn benjamn requested a review from StephenBarlow as a code owner May 25, 2022 17:46
@benjamn benjamn changed the base branch from release-3.6 to release-3.7 May 25, 2022 17:47
@benjamn benjamn merged commit 681c45a into release-3.7 May 25, 2022
@benjamn benjamn deleted the useFragment-hook branch May 25, 2022 17:57
This was referenced Aug 30, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: useFragment hook
6 participants