Skip to content

Enable cyclic types in ObjectType and InputObjectType #73

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

Closed
wants to merge 1 commit into from

Conversation

atrniv
Copy link
Contributor

@atrniv atrniv commented Nov 13, 2015

I've implemented the ability to allow the definition of cyclic field configuration. However I'm not too sure about the implementation style since the code seems to implement it in different ways for object, interface and input object. I've also added a simple test cases for the cyclic type in object field configurations. Let me know if there is something else I need to do.

… fix error with cyclic InputObject configurations

Signed-off-by: Johny Jose <[email protected]>
@sogko
Copy link
Member

sogko commented Nov 18, 2015

Hi @atrniv

Thanks for taking time to contribute to the project, appreciate it very much 👍🏻

You are right, currently the way those three GraphQL Types handle cyclic field definition are different:

  • graphql.Object and graphql.Interface has a AddFieldConfig()
  • graphql.InputObject uses a Thunk (ported from graphql-js)

To give a little background, go does not like cyclic declaration. So, the following object definition does not work:

func main() {
    // This won't work
    personType := graphql.NewObject(graphql.ObjectConfig{
        Name: "Person",
        Fields: graphql.FieldConfigMap{
            "name": &graphql.FieldConfig{
                Type: graphql.String,
            },
            "bestFriend": &graphql.FieldConfig{
                Type: personType, // personType has personType
            },
        },
    })
}

The current AddFieldConfig() implementation is one way to workaround that:

func main() {
    // create the GraphQL object first
    personType := graphql.NewObject(graphql.ObjectConfig{
        Name: "Person",
        Fields: graphql.FieldConfigMap{
            "name": &graphql.FieldConfig{
                Type: graphql.String,
            },
        },
    })

    // add the field config that refers to itself
    personType.AddFieldConfig("bestFriend", &graphql.FieldConfig{
        Type: personType,
    })
}

With this PR, it'll allow user to define cyclic field configuration like so:

    personType := graphql.NewObject(graphql.ObjectConfig{
        Name: "Person",
        Fields: (graphql.ObjectFieldMapThunk)(func() graphql.FieldConfigMap {
            return graphql.FieldConfigMap{
                "name": &graphql.FieldConfig{
                    Type: graphql.String,
                },
                "bestFriend": &graphql.Field{
                    Type: personType,
                },
            }
        },
    })

graphql.ObjectConfig.Fields now is an inline function that would be resolved later at runtime.


We can have a discussion here to see which direction we would like to take 👍🏻
Both ways are valid and we could go either way.

I prefer the thunk/inline function way, it would be easier to write schema definition in one big chunk and it'll be much closer to how one would write it with graphql-js

After #68 gets merged in, probably graphql.ObjectFieldMapThunk can be renamed to graphql.FieldsThunk
(Thunk seems a very JS thing though, any other suggestions?)

    personType := graphql.NewObject(graphql.ObjectConfig{
        Name: "Person",
        Fields: (graphql.FieldsThunk)(func() graphql.Fields {
            return graphql.Fields{
                "name": &graphql.Field{
                    Type: graphql.String,
                },
                "bestFriend": &graphql.Field{
                    Type: personType,
                },
            }
        },
    })

Either way, we would need to make the way to define cyclic config consistent.

Cheers!

/cc @chris-ramon

@chris-ramon
Copy link
Member

closing this one in favor of: #229 — thanks a lot @atrniv 👍

@chris-ramon chris-ramon closed this Aug 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants