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

Use Struct Field Resolver instead of Method #282

Merged
merged 3 commits into from
Feb 4, 2019

Conversation

0xSalman
Copy link
Contributor

This fixes #28. Essentially, it reduces lot of boilerplate code by making use of struct field as resolvers instead of creating methods for each struct field.

Now, methods are required only when

  1. a struct implements a GraphQL interface. In this case, you create methods for fields which overlap
  2. a struct exposes a GraphQL field that accepts additional arguments i.e., first: Int, last: Int etc.
  3. there is GraphQL union type

By using struct fields as resolvers, one could also benefit from DRY codebase by not having a pair of a struct which look the same (one for GraphQL and one for DB)

Does this break existing API?

No. This change is completely transparent and you can continue using your existing codebase.

Can I still continue using methods as resolvers?

Yes, by default, it uses method resolvers. Even when UseFieldResolvers() is turned on, the method is given higher priority than the field. When there is no method, the field will be used if there is any.

How can I use struct fields as resolvers?

When invoking graphql.ParseSchema() or graphql.MustParseSchema() to parse the schema, you pass in graphql.UseFieldResolvers() as an argument. For an example, take a look at ./example/social/server/server.go

Additional Notes

  • for an example, take a look at ./example/social/social.go & ./example/social/server/server.go
  • Unit tests are missing

@0xSalman
Copy link
Contributor Author

@pavelnikolov

I have implemented your PR review suggestions. Please take a look

@bazaglia

Your suggestion to use hybrid approach is implemented.

@bazaglia
Copy link

@pavelnikolov are you OK with merging this?

@nanozuki
Copy link

I think this is a really useful feature and can't wait to use it. What't the plan for merging?

@pavelnikolov
Copy link
Member

I'll review the PR shortly. I'm sorry for the delay on this.

@eveld
Copy link

eveld commented Nov 28, 2018

Can't wait to throw away half of my resolver code after this is merged! :)

@ghost
Copy link

ghost commented Jan 15, 2019

Any update for this PR be merged?

.gitignore Outdated Show resolved Hide resolved
@0xSalman
Copy link
Contributor Author

0xSalman commented Jan 17, 2019

@pavelnikolov I have addressed your review comment. Please merge this PR, thank you!

@leefernandes
Copy link

leefernandes commented Feb 1, 2019

Thank the gods, and @Salman-Ahmad I was just looking for this functionality. ❤️

It looks great on my end. I appreciate that if a field does not exist, but a method does then the method will be used, especially for backward compatibility. I would suggest that graphql.UseFieldResolvers() possibly become the default.

{
  get(limit: 3) {
    name
    color
  }
}
package gql

import (
	"context"

	"github.com/gorilla/mux"
	graphql "github.com/graph-gophers/graphql-go"
	"github.com/graph-gophers/graphql-go/relay"
)

type Record struct {
	Name string
}

func (r *Record) Color(ctx context.Context) string {
	return "blue"
}

type query struct{}

type Options struct {
	Limit int32
}

func (q *query) Get(ctx context.Context, options Options) (*Record, error) {
	return &Record{
		Name: "salman",
	}, nil
}

func AddEndpoint(r *mux.Router) {

	s := `
	schema {
		query: Query
	}
	interface Record {
		color: String!
		name: String!
	}
	type Query {
		get(limit: Int! = 5): Record
	}
`
	schema := graphql.MustParseSchema(s, &query{}, graphql.UseFieldResolvers())

	r.Handle("/query", &relay.Handler{Schema: schema})
}

@pavelnikolov
Copy link
Member

I would suggest that graphql.UseFieldResolvers() possibly become the default.

@itsleeowen , your suggestion would introduce a breaking change which I would really like to avoid. Instead, this feature will be opt-in.

@leefernandes
Copy link

leefernandes commented Feb 2, 2019

@pavelnikolov Cool, I'm curious what that breaking change is? On the surface it looks backward compatible, if you're using struct methods, those continue to work.

@pavelnikolov
Copy link
Member

@itsleeowen oh, my bad 🤦‍♂️
There is no breaking change. I got confused..

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@pavelnikolov pavelnikolov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this is ready to go in after a few minor changes...

example/social/server/server.go Outdated Show resolved Hide resolved
example/social/social.go Outdated Show resolved Hide resolved
graphql.go Outdated Show resolved Hide resolved
@pavelnikolov pavelnikolov merged commit e582242 into graph-gophers:master Feb 4, 2019
@pavelnikolov
Copy link
Member

Thank you for your contribution!

@tobstarr
Copy link

tobstarr commented Feb 8, 2019

🎉 just at the right time! Thank you so much!

@marcelvanwaaijen
Copy link

Question about the social example: What is the use of the FriendsResolver method? When is it called? I put some debug logging in it, but that never shows, also the pagination doesn't seem to be doing anything currently?

@pavelnikolov
Copy link
Member

@marcelvanwaaijen I haven't looked into it in details but my wild guess is that it was an existing example and when this feature was implemented we ended up adding graphql.UseFieldResolvers() which uses fields instead of methods. Then we started using the fields and stopped using the method. I would suggest opening a separate issue if this is the case as this one was related to adding the field resolvers feature, which is already done.

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.

Use field as resolver instead of method.
8 participants