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

General Design Questions Regarding Apollo #847

Closed
wax911 opened this issue Mar 5, 2018 · 3 comments
Closed

General Design Questions Regarding Apollo #847

wax911 opened this issue Mar 5, 2018 · 3 comments

Comments

@wax911
Copy link

wax911 commented Mar 5, 2018

Thank You For All The Hard Work
I've noticed some design choices which I'd like to ask about, or rather discuss. I'm by no means saying that the design choices made are bad 👍

Question 1:
I've noticed that the ApolloCall.Callback is a class object which exposes some useful methods which are not mandatory, I was wondering whether the Callback must really be a class or an interface. Would it not be better to have an interface for the abstract methods to handle response and failure to avoid clients having a long inheritance ladder?

Question 2:
Response classes types are generated automatically which is fine although modifying these classes to add support for parcels won't be possible unless if we extend the classes or duplicated the response data into a new class and implement the Parcelable interface.

Question 3:
Would it be possible to set primitive types rather than wrapper classes for generated classes with the gradle plugin?

Thank you in advance!

@ghost ghost added the feature label Mar 5, 2018
@sav007
Copy link
Contributor

sav007 commented Mar 7, 2018

Hi, good questions:

Q1: The real thing behind making callback to be abstract class rather than interface is this function:

public void onHttpError(@Nonnull ApolloHttpException e) {
      onFailure(e);
      okhttp3.Response response = e.rawResponse();
      if (response != null) {
        response.close();
      }
    }

Default implementation of this will ensure that okHttp response is closed. User can override potentially this function to parse error that been sent in error response body (that what many folks requested, and that's why callback became an abstract class not interface).

Q2: first of all Apollo can be used for regular Java projects and we didn't want to make runtime, code generation android specific. Second we intentionally don't suggest to pass graphql response as parcelable object between Activities, Fragments as the responses could potentially be a large and causing too large transaction exception on Android. We suggest of using view models instead, that designed just for UI interaction.

Q3: do you mean for custom GraphQL scalar type mappings?

@wax911
Copy link
Author

wax911 commented Mar 7, 2018

Thank you for the early response! sav007

Q1: I must've missed that somehow, thank you for clarifying that 👍

Q2: I see, keeping the view model in mind I see that the generated Data classes are all unique, meaning generics for common types can't be used, I was hoping that the classes for the same data type had some sort of base class. which would allow code reuse Polymorphic Type Handling but this features seems to be on hold

Q3: I somehow assumed that the Data objects of the GraphQL responses came were all wrapped even if they were non-null, sorry for that

Seems like the class generator has an issue with fragments with the same name in different .graphql files. e.g. I have some pagination fields which I just named pageInfo in more than one .graphql file and the only solution would be to make these inline fragments

Thanks again! Love the library 🌟

@sav007
Copy link
Contributor

sav007 commented Apr 4, 2018

Q2: you can't even imagine how this is complicated, as it involves fields merging, generating interfaces and confirming classes to them, then there is nullability issue etc. We think the effort of doing this is not reasonable for now.

Fragments are global scoped, as the idea to be able to share the same fragment between queries. So not sure what the issue you have.

@sav007 sav007 closed this as completed May 1, 2018
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

No branches or pull requests

2 participants