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.

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

Conversation

gsabran
Copy link

@gsabran gsabran commented Oct 22, 2019

This PR changes a few closures that were keeping a strong reference to the Apollo client. This is to make sure that as soon as the user of Apollo client releases its reference, all Apollo operations will stop.

Copy link
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

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

Good call on the closures, got a question on one thing though

@@ -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

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.

Copy link
Contributor

@designatednerd designatednerd left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@designatednerd designatednerd added this to the 0.18.0 milestone Oct 22, 2019
@designatednerd designatednerd merged commit 58e5dbe into apollographql:master Oct 22, 2019
@mitchellporter
Copy link

Anyone else who ends up here wondering why they're not getting any data back anymore... you're probably not holding onto a strong reference of the ApolloClient now that the retain cycle has been fixed.

@designatednerd
Copy link
Contributor

Ha, good catch! I'll make a note of it on the release notes for 0.18.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants