-
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
Add support for passing in client into useFragment #11525
Conversation
@vezaynk: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
👷 Deploy request for apollo-client-docs pending review.Visit the deploys page to approve it
|
🦋 Changeset detectedLatest commit: 1684e9c 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 |
Hey @vezaynk 👋 Appreciate the PR! Before we move forward with this, would you mind writing a couple tests for this? We want to be sure that future changes to our hooks don't break existing functionality. You can find the useFragment tests here: https://github.com/apollographql/apollo-client/blob/main/src/react/hooks/__tests__/useFragment.test.tsx If you'd like an example of how we test some of our other hooks that allow a apollo-client/src/react/hooks/__tests__/useSuspenseQuery.test.tsx Lines 992 to 1025 in 038629c
Assuming we move forward with this, we will likely slate this for our 3.10 release as its new functionality and we are in a code freeze for 3.9. 3.9 will be released as soon as we finish documentation, which should be early next week. Thank you! |
@jerelmiller Can this be shipped as a patch after the release? It's a very minor API extension that mirrors what every other hook does. It's not really "new functionality". I'm happy to put more work into this, but I would appreciate being able to download this from npm afterwards. |
@vezaynk I'll discuss with the team and let you know. We have a team meeting first thing Monday morning. In the meantime, if you need this functionality, I'd recommend using |
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.
Hey @vezaynk 👋
I'm still waiting on confirmation about when we'd like to release this. One of our maintainers has been out recently, so as soon as we get a consensus, I'll let you know.
In the mean time, I wanted to get you a proper review. Aside from adding tests, the only thing that I'd like to see is a docblock for the client
option.
Appreciate the contribution!
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
@jerelmiller Thanks for the review. I have added the suggestions and wrote 2 tests. |
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 adding those tests, @vezaynk! This is super close to merging - the team synced now that we're all back online and we'll get it out in the next patch 🚀
Added a few minor comments, one question for @phryneas to make sure the docblock is good to go (edit: we're good to go here) and the tests are failing on CI because they're missing one thing: client?: ApolloClient<any>;
needs to be added to the type test "UseFragmentOptions interface shape" at the bottom of useFragment.test.tsx
.
Co-authored-by: Alessia Bellisario <github@bellisar.io>
Co-authored-by: Alessia Bellisario <github@bellisar.io>
Hi @vezaynk 👋 there's still one test failing ("UseFragmentOptions interface shape" at the bottom of useFragment.test.tsx). Let me know if you were going to circle back to that, otherwise it's no problem for me to push up the one line fix and merge, thanks! |
Co-authored-by: Alessia Bellisario <github@bellisar.io>
@alessbell @jerelmiller I think I addressed everything |
Thanks so much @vezaynk! As soon as we see the tests pass, we'd be happy to merge this. Don't worry about the failed security scan checks or api extractor checks. We'll handle those 🙂 Thanks so much again for the contribution! |
This has been released in v3.9.3 🎉 |
My use-case requires passing in
client
explicitly to all hooks. This PR introduces support for it in useFragment, which was missing it.