-
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
feat: deep merge resolvers #11760
feat: deep merge resolvers #11760
Conversation
🦋 Changeset detectedLatest commit: c6eaea4 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 |
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
NB: |
Very curious that this test is consistently failing on CI (not locally) given the changes in this PR... haven't had a chance to dig deeper here. |
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.
Deep merging FTW! I love this!
I had a couple minor nits regarding the profiler, and one comment regarding the structure of the reset test, but otherwise looks good!
}, | ||
}, | ||
}); | ||
it("setup test where we add resolvers to schema", async () => { |
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.
While I totally understand why you setup these tests this way, I'm not super fond that these tests require a specific order in that this one needs to run before the other one to pass. Rather than separate it
blocks, I'd advocate for combining them into a single bigger test that demonstrates that calling .add
, then .reset
will successfully reset back to the original resolvers.
I think all I'd add in the component setup for your tests is a <button >
that calls refetch
on the useSuspenseQuery
that you can use to retrigger the network requests.
Thoughts?
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.
…hql/apollo-client into issue-11758-merging-resolvers
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.
🎉 awesome awesome stuff!
Closes #11758.