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

make parts of GraphQLGetTransformer and MultipartFormData public #1273

Merged
merged 1 commit into from
Jun 29, 2020
Merged

Conversation

ethansinjin
Copy link
Contributor

This allows creating NetworkTransport subclasses outside of the Apollo module that still make use of GraphQLGETTransformer and MultipartFormData.
A use case for doing this (and my actual use case) is replacing the use of URLSessionTask in HTTPNetworkTransport.swift with another networking library, such as Alamofire (ex. AlamofireNetworkTransport.swift)

This allows creating NetworkTransport subclasses outside of the Apollo module that still make use of GraphQLGETTransformer and MultipartFormData.
A use case for doing this would be replacing the use of URLSessionTask with another networking library, such as Alamofire.
@designatednerd
Copy link
Contributor

Hi! A couple questions:

  • What's your reason for wanting to subclass rather than making your own implementation of the NetworkTransport protocol?
  • Wouldn't we need to make HTTPNetworkTransport have an open annotation for this to work? Or is that implicit since it's not final?

@ethansinjin
Copy link
Contributor Author

ethansinjin commented Jun 29, 2020

Hey, thanks for the response! Sorry, I misspoke in my original message. I am indeed implementing the NetworkTransport protocol, not subclassing HTTPNetworkTransport. However, my class is effectively mirroring HTTPNetworkTransport, and only changing parts related to creating a URLSessionClient, URLSessionTask, and URLRequest. Because only those parts are changing, it would be helpful to use GraphQLGETTransformer and MultipartFormData in the same way that HTTPNetworkTransport's implementation does, to avoid having to reimplement those classes as well. At the moment, that can't be done due to the lack of public annotations on the classes themselves, as well as some of their properties. That's what this PR aims to resolve.

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.

Got it, thank you for the clarification. I think these are fine to make public. Thanks for the PR!

@designatednerd designatednerd added this to the Next Release milestone Jun 29, 2020
@designatednerd designatednerd merged commit 5c50a96 into apollographql:main Jun 29, 2020
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.

2 participants