Add public init for GraphQLResultReader for use with a JSONObject input

[ephemer]

For this PR to make sense it helps to understand our unusual use-case:

We have a React Native app in which we use the javascript Apollo Client to fetch data from our GraphQL server. One vital and performance-critical section of the app is an entirely native view though, and for that we use Apollo-iOS for two purposes:

1. An "out of the box" apollo-ios configuration for testing our native view in a standalone app (in a non-react native container). With apolloClient.fetch (in Swift)
2. To parse the same data collected by Apollo Client (JS) into native data types for the same native views within the React Native App (in production)

I just updated our Apollo iOS version to take advantage of the fix that maps GraphQLFloat -> Double, and to be on par with the latest version of apollo-codegen. I then found that our production use case was broken, due to the way XYZQuery.Data is now constructed via a Reader instead of a JSONMap.

This is a very simple change that re-enables the above use case without making any unneccesary changes to access control elsewhere in the code. It could also be interesting for other cases like loading GraphQL data from a cached JSON string etc. I noticed there was some discussion about using alternative NetworkTransports etc., maybe this would be a simple solution to those discussions too

[ephemer]

I'm not sure whether the query variables parameter is useful at all here (for us it makes no difference), I just wanted to keep the API similar to the existing init

[martijnwalraven]

That's an interesting use case! I'm glad you found out how to get this working with the recent changes to the parsing code.

For clarity, I think I'd prefer a rootObject parameter over jsonMap. The variables parameter is only used when generating the cache key for a field, so unless you want to use normalized caching it doesn't really matter what you pass in.

I'd be happy merging this, but alternatively you could add your convenience initializer in an extension in your own module, and that should also work.

[amol-c]

@martijnwalraven shouldn't init(variables: GraphQLMap? = [:], resolver: @escaping GraphQLResolver) be public to call it through extensions, incase we want to mock for unit testing ?

or am i missing out something ?

[martijnwalraven]

Ah, I hadn't realized it wasn't public already! I'd be fine making it public, and that might actually be better for extensibility than adding this convenience initializer.

Would that work for you?

I'm not sure what you mean by 'incase we want to mock for unit testing'?

[amol-c]

Yea. For my case, that would work.

We are using wrapper objects to wrap the auto generated models to add some custom logic.

To unit test the custom logic within the wrapper object, we need to mock the autogenerated model object.

The only way I see that happening with current apollo implementation , is through injecting 'GraphQLResultReader'.

Hope that makes sense.

[martijnwalraven]

Hmmm, could you use an extension to add this custom logic instead? (Like in this simple example).

That doesn't solve the need for a testing solution yet of course. I think the cleanest way would probably be to generate a public memberwise initializer for every model struct. Would that solve your issue?

[amol-c]

Yea. That would make life easier for unit testing.

I think the wrapper works better because the rest of the callers don't need to worry about the nested structs.

In my opinion, more than 2 level of nesting starts to obscure the code and starts making it difficult to read and reason about the same.

But, probably nesting might make sense for apollo given a graphql object contains a part of object and not the entirety. So to avoid name collision, apollo might need nesting.

[martijnwalraven]

Yeah, the reason for nesting is that every level of a query selects its own fields.

But I would recommend to use fragments for coherent selections of data (basically 'view models'), so those should help break the nesting and may be a good place to define custom logic on (also because they can be reused between different operations).

[ephemer]

@martijnwalraven I've renamed the parameter to rootObject and removed the variables parameter. I'm still not really sure what it was for, but it sounds like it's not particularly useful for inputs that don't come from the network.

---

To clarify the need for this PR: I'm not able to add the convenience initializer in an extension because it depends on internal methods and types, namely the normal GraphQLResultReader.init func (as mentioned elsewhere in this thread) and field.responseName.

Personally I have no need to use those internal parts for anything else, so I'd find it cleaner to just deal with a public convenience init.

[martijnwalraven]

Ok, merged!

[ephemer]

Great, thanks!