Resolve params rebase

[anmonteiro]

This is #82 and https://github.com/andreas/ocaml-graphql-server/tree/resolve-params rebased on current master.

It adds both variables and fragments to the resolve params. I could imagine this having more fields just like in the GraphQL reference implementation:

https://github.com/graphql/graphql-js/blob/26c9874107e65f19576aae0a32638287820f68aa/src/execution/execute.js#L97-L106

[sgrove]

Seems like this would also solve #82 as well, right?

[anmonteiro]

I think that was also my intent

…

On Nov 16, 2018, at 3:03 PM, Sean Grove ***@***.***> wrote: Seems like this would also solve #82 as well, right? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[ozanmakes]

I needed something like this and this branch worked very well for me! Are there any plans to get this merged?

[andreas]

I think there's a definite need for exposing more context to resolvers, e.g. the field, variables and fragments as in this PR. I'm wondering how to do this best though.

From an ergonomics-perspective, it's nice that resolve_params is not an abstract type, but it makes it harder to change the implementation going forward.
On the other hand, if resolve_params is abstract, it's a little more heavy-handed to use. Example:

Schema.(field "foo"
  ~typ:string
  ~args:[]
  ~resolve:(fun params src ->
    let ctx = ResolveParams.ctx params in
    (* ... *)
  )
)

Also, to align with graphql-js, should the name be resolve_info rather than resolve_params (definition.js)? Is it worth bikeshedding over what fields to include in this type?

[anmonteiro]

@andreas I think there's definitely some room for improvement, and I would agree that it's important to get right initially what we wanna expose.

Happy to bikeshed on naming + what fields can go in.

On the other hand, if resolve_params is abstract, it's a little more heavy-handed to use. Example:
I don't think making resolve_params abstract is worth the tradeoffs.
The only disadvantage I'm seeing is that if we add fields the type of resolve_params changes. But if we're only adding fields that isn't really a problem, or is it?

[andreas]

Happy to bikeshed on naming + what fields can go in.

I like the name resolve_info, so let's go for that if you agree. I've taken a second look at the fields in graphql-js, and I think the fields you've picked out so far is a good starting point. Are there any of the fields you think we should include at this point?

The only disadvantage I'm seeing is that if we add fields the type of resolve_params changes. But if we're only adding fields that isn't really a problem, or is it?

Adding a field can in theory be a problem too, but I think it's a minority use case that we can mostly disregard. Changing a field is obviously a problem too..

[anmonteiro]

I like the name resolve_info, so let's go for that if you agree. I've taken a second look at the fields in graphql-js, and I think the fields you've picked out so far is a good starting point. Are there any of the fields you think we should include at this point?

Will do. I don't any other fields I'd like to include at this point. I also looked at graphql-js to see which ones they had when I made the PR.

Adding a field can in theory be a problem too, but I think it's a minority use case that we can mostly disregard. Changing a field is obviously a problem too..

I didn't list changing a field because that would be a problem in the abstract case too (in case the type changed – if the name of the field changes having an abstract type is an obvious advantage here)

[anmonteiro]

Just rebased this on master and changed the name from resolve_params to resolve_info.

[andreas]

Looks good. Could you update README.md and examples/server.ml as well please?

[andreas]

Nit: _info

[anmonteiro]

@andreas nice catch. Done

[andreas]

Sorry for the nitpicking and not being clear in my previous comment: I'd like the idiom to be fun info src -> ... for resolve functions, i.e. the argument name should be called info instead of resolve_info in the README and examples.

[anmonteiro]

That's fine. I'll change it right now.

[anmonteiro]

Done

[andreas]

Perfect, thanks!