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

Allow setting the delegate property on transport #685

Closed
wants to merge 1 commit into from

Conversation

@koenpunt
Copy link

commented Aug 1, 2019

In addition to passing the delegate as an argument to HTTPNetworkTransport, it's practical to be able to set it as a property as well.

For example; we use a wrapper class for the client, which looks something like this:

class GraphQLClient {
    static var shared = GraphQLClient(url: Config.API.graphQLURL)
    private let client: ApolloClient
    init(url: URL) {
        let transport = HTTPNetworkTransport(url: url,
                                             configuration: URLSessionConfiguration.default,
                                             sendOperationIdentifiers: false,
                                             delegate: self)

        self.client = ApolloClient(networkTransport: transport)
    }
}

But passing self to HTTPNetworkTransport as delegate doesn't work, because that gives the compile time error:

Constant 'self.client' used before being initialized

From inspecting the code it should be totally safe to make the delegate property public and assignable, which would allow the delegate to be set after self.client is initialized;

class GraphQLClient {
    static var shared = GraphQLClient(url: Config.API.graphQLURL)
    private let client: ApolloClient
    init(url: URL) {
        let transport = HTTPNetworkTransport(url: url,
                                             configuration: URLSessionConfiguration.default,
                                             sendOperationIdentifiers: false)

        self.client = ApolloClient(networkTransport: transport)

        transport.delegate = self
    }
}
Allow setting the delegate property on transport
In addition to passing the delegate as an argument to HTTPNetworkTransport, it's practical to be able to set it as a property as well.
@designatednerd

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Hm - why not do this with a lazy initializer? Like:

class GraphQLClient {
    static var shared = GraphQLClient()
    private lazy var client: ApolloClient = {
        let transport = HTTPNetworkTransport(url: Config.API.graphQLURL,
                                             configuration: URLSessionConfiguration.default,
                                             sendOperationIdentifiers: false,
                                             delegate: self)

        return ApolloClient(networkTransport: transport)
    }()

   // Now you no longer need the initializer if the ApolloClient was the
   // only thing you were setting up in it.
}

Particularly if the delegate isn't going to be changing through the lifecycle of the ApolloClient (which it really shouldn't), I'm not a giant fan of exposing it publicly.

What do you think?

@koenpunt

This comment has been minimized.

Copy link
Author

commented Aug 1, 2019

This works for the implementation, but makes testing really hard, because the url is no longer an argument to the class.

Also having a delegate as property is a very common thing, what would be the disadvantage of exposing it publicly? And why shouldn't it be changed during the lifecycle of the client? Not that I currently am, or am planning to, but I don't really see any harm in doing it.

For now I've resolved the issue by making the client property an implicitly unwrapped optional, but I'm not a fan of that.

@koenpunt

This comment has been minimized.

Copy link
Author

commented Aug 1, 2019

And why shouldn't it be changed during the lifecycle of the client?

An potential use case of using multiple/different delegates would be when for an authentication query you want to use a different retry mechanism than for regular queries, which then can easily be achieved by swapping the delegate. But this is purely hypothetical.

@designatednerd

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Ah, I figured you were using that Config to set up the URL. You can also do this:

class GraphQLClient {
    static var shared = GraphQLClient(url: Config.API.graphQLURL)
    private let url: URL
    private lazy var client: ApolloClient = {
        let transport = HTTPNetworkTransport(url: self.url,
                                             configuration: URLSessionConfiguration.default,
                                             sendOperationIdentifiers: false,
                                             delegate: self)

        return ApolloClient(networkTransport: transport)
    }()

   init(url: URL) {
     self.url = url
   }
}

The biggest concern I have on swapping out the delegate mid-stream is that it's possible to have multiple in-flight requests that can come back in an indeterminate order. Swapping the delegate at runtime is an awesome way to have all of that blow up in your face if you swap it too early or too late.

The example you gave about auth vs. other types of retry are the reason why the original request is included as part of the retry callback. That way, you can decide how to route the decision to retry to wherever you want based on which request is failing and why.

@koenpunt

This comment has been minimized.

Copy link
Author

commented Aug 1, 2019

Alright thanks for the explanation, and for the suggestion. Seems like a good approach :)

@koenpunt koenpunt closed this Aug 1, 2019

@koenpunt koenpunt deleted the koenpunt:patch-2 branch Aug 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.