-
Notifications
You must be signed in to change notification settings - Fork 36
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
0.9.0 (2) - wrap hook functionality from ApolloClient instance #216
Conversation
export const useReadQuery = wrap(orig_useReadQuery, ["data", "networkStatus"]); | ||
|
||
export const useBackgroundQuery = orig_useBackgroundQuery; | ||
export const hookWrappers: HookWrappers = { |
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.
We'll never get our "wrapping" code to tree-shake, but that's okay for me.
What's important here is that the upstream hooks can shake out if they are not used.
@@ -7,7 +7,7 @@ export { | |||
useSuspenseQuery, | |||
useReadQuery, | |||
useBackgroundQuery, | |||
} from "./hooks.js"; | |||
} from "@apollo/client/index.js"; |
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.
No more custom implementations & it doesn't matter where it's imported from 🎉
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'm curious, is there any reason not to just deprecate these exports and have users import directly from @apollo/client
? That could simplify things even further 🙂
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.
Removing it from this package, but keeping it in the "outer" package for now. (Will drop it in a future version.)
} | ||
|
||
[wrappers] = hookWrappers; |
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.
See https://github.com/apollographql/apollo-client/pull/11617/files#r1500875764
I'd be curious if we could store this on QueryManager
which would make a path for useReadQuery
to work in all scenarios.
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 like this a lot! Really creative solution to allow for more simplification.
ce940ef
to
12b6ff3
Compare
This PR builds on apollographql/apollo-client#11617 and does the same as #215 or #206, but should tree-shake better than #215 and has better UX than #206