-
Notifications
You must be signed in to change notification settings - Fork 730
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
Create a protocol for public methods of Apollo client #693
Conversation
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.
Really like the idea in general but there's a "fun" corner of POP that I think will bite us here.
public extension ApolloClientProtocol { | ||
|
||
@discardableResult func fetch<Query: GraphQLQuery>(query: Query, cachePolicy: CachePolicy = .returnCacheDataElseFetch, context: UnsafeMutableRawPointer? = nil, queue: DispatchQueue = DispatchQueue.main, resultHandler: GraphQLResultHandler<Query.Data>? = nil) -> Cancellable { | ||
return fetch(query: query, cachePolicy: cachePolicy, context: context, queue: queue, resultHandler: resultHandler) |
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.
One thing to watch out for with these default implementations is that they will override the local implementation anywhere that the client is passed as ApolloClientProtocol
. This is an extremely oversimplified example of that behavior.
I would recommend removing the default implementations here and just setting the default params on the actual implementations, since it seems like the default implementations are just there to re-add the default parameters.
@gsabran Would love to get this in with |
Hey @gsabran - If I don't hear back from you by tomorrow, I'm going to take over this PR so I can get it shipped. Excellent idea! |
Closing in favor of #715 since there hasn't been any movement on this and I'd still like to get it out with |
Hi thanks a lot for taking over, and sorry for not following up at all.. I left for vacations :) |
Haha, yep, I figured. 'Tis August after all. Thanks for the idea! |
This is so that developers can refer to the protocol instead of the concrete type, and use dependency injection for unit testing. It doesn't break any public APIs.