-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
change UseFragmentResult
to union type
#10765
Conversation
🦋 Changeset detectedLatest commit: 930623f 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 |
@@ -115,7 +121,7 @@ function diffToResult<TData>( | |||
diff: Cache.DiffResult<TData>, | |||
): UseFragmentResult<TData> { | |||
const result: UseFragmentResult<TData> = { | |||
data: diff.result, | |||
data: diff.result!, |
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.
From what I saw in the tests, this will never be undefined
, but come out as {}
- I adjusted the types accordingly.
@benjamn, can you please take a look here to make sure I didn't forget about some edge case?
@@ -349,9 +350,10 @@ describe("useFragment", () => { | |||
from: { __typename: "Query" }, | |||
}); | |||
expect(complete).toBe(true); | |||
assert(!!complete) |
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.
the assert
call is here so TypeScript knows that data
is complete in the JSX.
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.
Had a couple minor suggestions. Thanks for getting this updated!
src/react/hooks/useFragment.ts
Outdated
missing?: MissingTree; | ||
} | ||
| { | ||
data: Partial<TData>; |
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.
Should we use a DeepPartial
here since you can give a fragment that could contain nested field selections?
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.
Good point. We don't have a DeepPartial
type at this point yet though - and the one added with #10766 will land in 3.8. We should coordinate around that.
f65bf2f
to
d4e3f0a
Compare
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
@jerelmiller this now uses |
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.
Thanks for updating to DeepPartial
! These types look great!
This should be merged after #10764 and is the second half of suggestions from #10760 and would address #10650.
It also adds a test to make sure the types actually reflect the real behaviour.
Checklist: