narrow return value of useFragment based on returnPartialData option

[phryneas]

This would address the first point raised over in #10650

Checklist:

• If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
• If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests

[changeset-bot]

🦋 Changeset detected

Latest commit: c0266b7

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

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Patch

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

[alessbell]

Nice! LGTM ✅

I also like the idea of introducing some utility types to make writing this kind of test easier (e.g. https://github.com/total-typescript/ts-reset/blob/main/src/tests/utils.ts), but since we don't have anything like that yet, @ts-expect-error comments do the trick :)

Edit: just noticed the useFragment tests will need some updates with this change.

[benjamn]

I'm glad to see this works, but it's tempting to fully remove the returnPartialData option from useFragment, and let result.complete and result.missing indicate whether the result.data is partial or complete.

What do you think about that simplification? If we eliminate returnPartialData, that might solve the second bullet point from #10650 as well…

Edit: same comment as @Alessia about fixing those useFragment tests

[phryneas]

Yeah, tests on the branch seemed generally broken, so I had not given much thought to CI results. I can try to fix that tomorrow - and move this over to 3.7.

As for removing the returnPartialData, that could also be a way to go. I would argue for keeping (and fixing) it though:

Essentially we have to choose between these two:

const result = useFragment({
  fragment: typedNode,
  from: { __typename: "Query" },
  returnPartialData: false,
});

if (result.data) {
  // ...
}

and

const result = useFragment({
  fragment: typedNode,
  from: { __typename: "Query" },
});

if (result.complete) {
  // ...
}

I would argue that once result.data is passed in somewhere independently of result.complete, it cannot be checked any more if the type is partial or not - while it can be checked if it is present.

So further down the line we would have one of these two scenarios:

function doSomeThing(data: TData | undefined) {
  if (!data) return;
  // data is TData now
}

and

function doSomeThing(data: Partial<TData> | undefined) {
  // we have no way of knowing here if `TData` is complete
}

I would personally prefer the first option here.

[jerelmiller]

@phryneas if we use a discriminated union as such:

// this isn't the complete type, but it illustrates the point
type UseFragmentResult<TData> = 
  | { complete: true, data: TData } // note `data` is not `undefined` in this case
  | { complete: false, data: Partial<TData> | undefined }

Wouldn't you still be able to achieve the same type of thing with doSomeThing if you switch the signatures around a bit?

function doSomeThing(data: TData) {
  // only accept `TData
}

if (result.complete) {
  // result.data is now `TData` according to the discriminated union
  doSomeThing(result.data)
}

---

I personally like to keep the surface area of the API as small as possible, so one less option is attractive to me, especially if the return value captures whether the data is partial or complete.

[phryneas]

Yes, but I had to do the if (result.complete) immediately (while I still have access to result).
The other way, data would be enough information to make checks on it. That might especially be useful where that if would lead to "weird" code, like in JSX.

Of course the whole thing could be simulated in userland by doing something like

const result = useFragment({
  fragment: typedNode,
  from: { __typename: "Query" },
});
const data = result.complete ? result.data : undefined

So it would not be the end of the world, just a bit less convenient 🤔.

So either way, I probably don't have a very strong opinion here :)

[alessbell]

My $0.02: I see the merits in both approaches (there can be an ergonomic advantage to data having its own way to signal completeness with returnPartialData: false, as you've shown), but I've come around to preferring the smaller API surface area + semantics of result.complete here. #10764 & #10765 LGTM 🎉