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

Implement graphql.Do as described in #44 #62

Closed
wants to merge 1 commit into from
Closed

Implement graphql.Do as described in #44 #62

wants to merge 1 commit into from

Conversation

FugiTech
Copy link
Contributor

@FugiTech FugiTech commented Nov 6, 2015

This is an attempt to simplify graphql.Do as described in #44. While I greatly enjoy not having to pass in a struct, this appears to remove ability for the user to define RootValue, OperationName and VariableValues. I'm not sure if the struct or just having five arguments is better. Maybe compromise with having a wrapper around Do that only takes two arguments for the simple case? I don't know how common it'll be in practice.

This is expected to not pass tests, as I haven't yet updated all the tests to the new API. I'm holding off on that until it's determined what API is ideal.

@sogko
Copy link
Member

sogko commented Nov 6, 2015

Hi @Fugiman

Thanks for working on this PR 👍🏻

Having graphql.Do(schema, query) does make it look very attractive to use.

I think we still need to pass in graphql.Params, because it might not be enough to just have schema and query.
There are still valid cases for passing in root value, variable values and operation name.

I wish Go has named arguments right now, or at least remove the need to explicitly write the struct type for functions, similar to map literals. (There are a couple of proposals floating around, but it's safe to say that it won't be here anytime soon).

It would be nice to have

result := graphql.Do({Schema: schema, Request: request})

Writing all five arguments may not be the best for a couple of reasons:

  • Only schema and request are essential, the others are optional. This may lead to graphql.Do(schema, query, nil, nil, nil) I don't know if thats any better
  • Adding a new argument would essentially be very disruptive and break compatibility very easily, as opposed to just added a field to the Params struct.

But your suggestion of creating a wrapper does sound reasonable. Though I can't think of a good name for it right now lol.

Cheers!

/cc @chris-ramon

@chris-ramon
Copy link
Member

Thanks for going ahead and start working on this @Fugiman! 🌟

I would like to mention that we might consider other possible names for graphql.Graphl function, I've consider the following:

  1. graphql.Do
  2. graphql.Run
  3. graphql.Exec
  4. graphql.Execute

graphql.Graphql in summary does: Source -> Parse -> AST -> Validate -> Execute -> Result

Said that, multiple steps and components are involved, so perhaps graphql.Execute and graphql.Exec might not be appropriate.

graphql.Run - gives the notion of continuos execution.
graphql.Do - gives the notion of sync execution.

graphql.Graphql was recently updated to be sync, so renaming to graphql.Do seems to be convenient plus is go-idiomatic.

So please do share ur thoughts on this.


How other GraphQL implementations are naming graphql.Graphql:

  • github.com/rmosolgo/graphql-ruby -> Schema.execute
  • github.com/sangria-graphql/sangria -> Executor.execute
  • github.com/graphql/graphql-js -> graphql
  • github.com/graphql-python/graphene -> schema.execute
  • github.com/andimarek/graphql-java -> GraphQL.execute

@FugiTech
Copy link
Contributor Author

FugiTech commented Nov 8, 2015

Closing in favor of #68

@FugiTech FugiTech closed this Nov 8, 2015
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

Successfully merging this pull request may close these issues.

3 participants