API Improvement. about graphql HOT 15 CLOSED

Woah the improvements starting to look good 👍🏻

My humble notes:

1. Resolve function

• graphql.ResolveParams is a great improvement over graphql.GQLFRParams👍🏻
• There is a proposal in issue #28 to allow user functions to return error instead of forcing them to panic.
  If any, executor will then return nil and add the user-returned error into the graphql.Result

    helloResolve := func(p graphql.ResolveParams) (interface{}, error) {
        return "world", nil
    }

2. graphql.Fields, graphql.Field

This is definitely a much need improvement (i.e. renaming FieldConfigMap and FieldConfig.

In the proposed example, it is a []graphql.Fields{} slice.

How about we retain it as map? Because the field key and the field name can be different.
(The name config is used in the introspection query and for documentation)

    package graphql
    type Field struct
    type Fields map[string]*Field
    ...
    package example
    fields := graphql.Fields{
        "hello": &graphql.Field{Name: "Hello Field", Type: graphql.String, Resolve: helloResolve},
    }

3. graphql.NewSchema() and root query

At first glance, the new NewSchema() method looks cleaner. But it might be veering too much from graphql. The proposed example looks like it can only define a root object for query operation (ie. RootQuery). We still need a way to define mutation root query.

A GraphQL schema is represented by a root type for each kind of operation: query and mutation; this determines the place in the type system where those operations begin.
Source: http://facebook.github.io/graphql/#sec-Type-System

func main() {
    helloResolve := func(p graphql.ResolveParams) interface{} {
        return "world"
    }
    queryFields := graphql.Fields{
        "hello": graphql.Field{Name: "Hello Field", Type: graphql.String, Resolve: helloResolve},
    }
    rootQuery := graphql.NewObject(graphql.ObjectConfig{
        Name: "RootQuery",
        Fields: queryFields,
    })
    schema, err := graphql.NewSchema(graphql.SchemaConfig{Query: rootQuery})
    // or with mutation, 
    // schema, err := graphql.NewSchema(graphql.SchemaConfig{Query: rootQuery, Mutation: mutationQuery})
    if err != nil {
        log.Fatalf("failed to create new schema, error: %v", err)
    }
    ...
}

4. Constructor configs

I wish we can get rid of the config structs for the constructor. There is some hope; there is a proposal for named arguments to be added to the language golang/go#12296 and golang/go#12854, but realistically, it would take awhile.

5. graphql.Do()

I'm in favour of this over the current go graphql.Graphql(...) 👍🏻
Your example shows go graphql.Do(schema, query, result); are you suggesting we replace the graphql.Params?

Cheers!

from graphql.

I think that

result := make(chan *graphql.Result)
go graphql.Do(schema, query, result)
r := <-result

is a very complicated interface. As far as I can tell, there's no need for the result channel at all (everything in the executor appears to be synchronous), but if it is required then it'd be nice for graphql.Do to handle creating the channel and waiting for the result. In other words,

r := graphql.Do(schema, query)

would be my ideal interface.

from graphql.

I also prefer the Do syntax.
Even the Fields slice seems easier for typing those long types.

from graphql.

Update: @Fugiman submitted PR #48 to improve the graphql.Graphql(...)
Updateupdate: Merged 👍🏻

from graphql.

Definitely looking forward to more robust error handling and less panic() at the disco :)

from graphql.

Hi @sogko I would like to get support on custom error reporting as #74

I can think of two approaches

1. change Resolve function signature to indicate errror as well

 helloResolve := func(p graphql.ResolveParams) (interface{}, error) {
        return "world", nil
    }

This is explained by @sogko above
2. Check returned value is an error interface in executor
http://play.golang.org/p/xoPSMAbEAB

package main

import "fmt"

func Resolve(isErr bool) interface{} {
    if isErr {
        return fmt.Errorf("got an error")
    }
    return 2 + 1
}

func assert(res interface{}) {
    if err, ok := res.(error); ok  {
        fmt.Println(err.Error())

    } else {
        fmt.Printf("got value %d", res)
    }
}

func main() {
    assert(Resolve(true))
    assert(Resolve(false))
}

//returns
got an error
got value 3

tradeoff
Option 1 has the benefit of being explicit and idiomatic in go sense. At the same time need API change.
Option 2 has the benefit of no API change but retuned result has some implicit meaning.

I prefer option 1 and being explicit. I can work on this if you all agree.

Cheers,
bsr

from graphql.

@chris-ramon @sogko any comment on error handling. currently the framework is catching panic and suppressing the cause of error. I keep hitting this as Iost context information make it hard to track down an error. Please give some priority on this.

from graphql.

Right now what I do is comment out the defers and recovers in executor.go that really helps and crashes your server a lot. But then that is only for development. We are actually planning to make a graphql implementation using code generation for performance in the future.

from graphql.

@pyros2097 with code generation? tell us more :)

from graphql.

By we I mean my company. Well we have 3 Type systems one was our postgres types, then go types and then graphql types. So to map go types to postgres types and tables we had done it using code generation using go generate. But didn't have time to do it for graphql types also. So say you have a struct

type User struct {
  ID string `json:"id"`
  name string `json:"id"`
}

we had to manually create a graphql type for each struct and that was a bit painful as we already know what types ID and name were and even needed to create custom scalar types for validation like PLPassword(which I think should be done in Go and on the struct). So you techincally wouldn't need to create all graphql types, maybe you might only need to resolve the fields.

And another thing we found out was that a lot of reflection was used for serialization and de-serialization and the current graphql implementation we need to resolve interfaces or map[string]interfaces{} which can be improved if you know the type you need to json encode.
We are planning to have some thing that can do something like a rails orm does for you but with also graphql types ex: sql -> structs -> graphql objects.
And by the way we have recently finished 2 projects and are currently testing it out.
Awesome work @sogko and @chris-ramon thanks to you guys we didn't need write an rpc layer between go and nodejs 👍

@atrniv Any word on the implementation you were talking about? (PS He is the code generation guy you can look at all his projects they all have code generation in them even the js ones )

from graphql.

@pyros2097 thanks for the tip. that helps :-)

from graphql.

Thanks for sharing your thoughts and concerns about api/lib design/improvement guys! 🌟

I've started working on extending graphql.FieldResolveFn to return error as second parameter: (interface{}, error), as @bsr203 mentioned above, work under progress on: #80

from graphql.

@chris-ramon you rock :-) I can see the effort you putting in this. tx.
Can you also look at #65 with this as you may be touching the same functions :-) Even if we don't use context, just being consistent is enough now map[string]interface{} -> interface{}

from graphql.

@pkieltyka We have implemented it and using it on our live servers you can check the tests out although there is no documentation whatsoever ... https://github.com/playlyfe/go-graphql

from graphql.

Let's close this one; and have a new conversation on API improvements on new issues, thanks a lot for bringing great ideas guys 👍 🌟

from graphql.