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 GraphQLResponse a generic on the response type instead of on the operation type #1061

Merged
merged 3 commits into from
Mar 5, 2020

Conversation

gsabran
Copy link

@gsabran gsabran commented Mar 4, 2020

It looks like there's little reasons to have GraphQLResponse be tied to the operation type, instead of more simply to the response type (.Data)

I was interested in this change as I use a query wrapper class around the GraphQLOperation generated by the codegen. This wrapper allows me to easily set context for my NetworkTransport that receives the operation through Apollo. While it shares the same response type as the wrapped operation, it is of a different type and this was not playing well with the GraphQLResponse

This slightly changes the public API of NetworkTransport as well, but all the changes are straightforward.

@designatednerd
Copy link
Contributor

While it shares the same response type as the wrapped operation, it is of a different type and this was not playing well with the GraphQLResponse

Can you clarify a bit what you mean by this, perhaps by giving an example?

This looks like a reasonably straightforward changeset, but it is definitely breaking so I'd like to mull it over a little. In the meantime, do you mind fixing the alignment of parameters in the method declarations that broke because you changed the name of the generic type?

@gsabran
Copy link
Author

gsabran commented Mar 4, 2020

Sure. At a high level the wrapper strategy looks like:

// networking service
class NetworkingService {
  init() {
    self.apolloClient = ApolloClient(transport: NetworkTransport())
  }

  func fetch(query: GraphQLQuery, requestConfiguration: RequestConfiguration, completion: ...) {
    // wrappedQuery is GraphQLQuery with the same Data type, but of different type than query
    let wrappedQuery = WrappedQuery(query, requestConfiguration)
    apolloClient.fetch(wrappedQuery, completion)
  }
}

class NetworkTransport {
  // called from Apollo
  func send(query: GraphQLQuery, completion: ...) {
    let requestConfiguration = (query as? WrappedQuery)?.requestConfiguration
    let request = buildRequest(query.operationName, query.params, requestConfiguration)
    apiSession.send(request, completion)
  }
}

It makes it easy to pass whichever context or handles I'm interested in, without having to change Apollo

@gsabran gsabran force-pushed the gui--response-data branch from 308992e to d9270df Compare March 4, 2020 01:43
@designatednerd
Copy link
Contributor

Ahhh ok so this is enabling a custom network transport, that's the piece I was missing in my mental model.

@designatednerd
Copy link
Contributor

Updated code looks good, I do want to think this over a bit since it'd be a breaking change.

@designatednerd designatednerd added this to the Next Release milestone Mar 5, 2020
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.

Going to put this out with the next release.

@designatednerd designatednerd merged commit cadc61b into apollographql:master Mar 5, 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