Skip to content

Use weak references to Apollo client in closures#854

Merged
designatednerd merged 1 commit intoapollographql:masterfrom
gsabran:gui--weak-ref-to-apollo-client
Oct 22, 2019
Merged

Use weak references to Apollo client in closures#854
designatednerd merged 1 commit intoapollographql:masterfrom
gsabran:gui--weak-ref-to-apollo-client

Conversation

@gsabran
Copy link
Copy Markdown

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


private final class FetchQueryOperation<Query: GraphQLQuery>: AsynchronousOperation, Cancellable {
let client: ApolloClient
weak var client: ApolloClient?
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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
Copy Markdown
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