Skip to content
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.

Sign up for GitHub

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

Use weak references to Apollo client in closures #854

Merged
merged 1 commit into from
Oct 22, 2019
Merged

Use weak references to Apollo client in closures #854

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 15 additions & 6 deletions Sources/Apollo/ApolloClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ public class ApolloClient {
}

fileprivate func send<Operation: GraphQLOperation>(operation: Operation, shouldPublishResultToStore: Bool, context: UnsafeMutableRawPointer?, resultHandler: @escaping GraphQLResultHandler<Operation.Data>) -> Cancellable {
return networkTransport.send(operation: operation) { result in
return networkTransport.send(operation: operation) { [weak self] result in
guard let self = self else {
return
}
self.handleOperationResult(shouldPublishResultToStore: shouldPublishResultToStore,
context: context,
result,
Expand All @@ -90,7 +93,10 @@ public class ApolloClient {

firstly {
try response.parseResult(cacheKeyForObject: self.cacheKeyForObject)
}.andThen { (result, records) in
}.andThen { [weak self] (result, records) in
guard let self = self else {
return
}
if let records = records {
self.store.publish(records: records, context: context).catch { error in
preconditionFailure(String(describing: error))
Expand Down Expand Up @@ -175,7 +181,10 @@ extension ApolloClient: ApolloClientProtocol {
return EmptyCancellable()
}

return uploadingTransport.upload(operation: operation, files: files) { result in
return uploadingTransport.upload(operation: operation, files: files) { [weak self] result in
guard let self = self else {
return
}
self.handleOperationResult(shouldPublishResultToStore: true,
context: context, result,
resultHandler: wrappedHandler)
Expand Down Expand Up @@ -206,7 +215,7 @@ private func wrapResultHandler<Data>(_ resultHandler: GraphQLResultHandler<Data>
}

private final class FetchQueryOperation<Query: GraphQLQuery>: AsynchronousOperation, Cancellable {
let client: ApolloClient
weak var client: ApolloClient?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to be weak here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're adding the operation to the client's queue

apollo-ios/Sources/Apollo/ApolloClient.swift

Lines 137 to 138 in 509b222

let operation = FetchQueryOperation(client: self, query: query, cachePolicy: cachePolicy, context: context, resultHandler: resultHandler)
self.operationQueue.addOperation(operation)

so both the client and the operation will be retained here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh woof, good point.

let query: Query
let cachePolicy: CachePolicy
let context: UnsafeMutableRawPointer?
Expand Down Expand Up @@ -235,7 +244,7 @@ private final class FetchQueryOperation<Query: GraphQLQuery>: AsynchronousOperat
return
}

client.store.load(query: query) { result in
client?.store.load(query: query) { result in
if self.isCancelled {
self.state = .finished
return
Expand All @@ -262,7 +271,7 @@ private final class FetchQueryOperation<Query: GraphQLQuery>: AsynchronousOperat
}

func fetchFromNetwork() {
networkTask = client.send(operation: query, shouldPublishResultToStore: true, context: context) { result in
networkTask = client?.send(operation: query, shouldPublishResultToStore: true, context: context) { result in
self.resultHandler(result)
self.state = .finished
return
Expand Down