graphql-go

graphql-go copied to clipboard

See https://github.com/graph-gophers/graphql-go/issues/145

github.com/graph-gophers/graphql-go (download)
github.com/opentracing/opentracing-go (download)
Fetching https://golang.org/x/net/context?go-get=1
Parsing meta tags from https://golang.org/x/net/context?go-get=1 (status code 200)
get "golang.org/x/net/context": found meta tag main.metaImport{Prefix:"golang.org/x/net", VCS:"git", RepoRoot:"https://go.googlesource.com/net"} at https://golang.org/x/net/context?go-get=1
get "golang.org/x/net/context": verifying non-authoritative meta tag
Fetching https://golang.org/x/net?go-get=1
Parsing meta tags from https://golang.org/x/net?go-get=1 (status code 200)
golang.org/x/net (download)
graphql.go:10:2: use of internal package not allowed
graphql.go:11:2: use of internal package not allowed
graphql.go:12:2: use of internal package not allowed
graphql.go:13:2: use of internal package not allowed
graphql.go:14:2: use of internal package not allowed
graphql.go:15:2: use of internal package not allowed
graphql.go:16:2: use of internal package not allowed
internal/exec/exec.go:11:2: use of internal package not allowed
internal/exec/exec.go:12:2: use of internal package not allowed
internal/exec/exec.go:13:2: use of internal package not allowed
internal/exec/exec.go:14:2: use of internal package not allowed
internal/exec/exec.go:15:2: use of internal package not allowed
internal/exec/packer/packer.go:10:2: use of internal package not allowed
internal/exec/packer/packer.go:11:2: use of internal package not allowed
internal/exec/resolvable/meta.go:7:2: use of internal package not allowed
internal/exec/resolvable/resolvable.go:12:2: use of internal package not allowed
internal/exec/resolvable/meta.go:8:2: use of internal package not allowed
internal/exec/selected/selected.go:9:2: use of internal package not allowed
internal/exec/selected/selected.go:10:2: use of internal package not allowed
internal/exec/selected/selected.go:11:2: use of internal package not allowed
internal/exec/selected/selected.go:12:2: use of internal package not allowed
internal/exec/selected/selected.go:13:2: use of internal package not allowed
internal/query/query.go:9:2: use of internal package not allowed
internal/schema/schema.go:9:2: use of internal package not allowed
internal/validation/validation.go:12:2: use of internal package not allowed
internal/validation/validation.go:13:2: use of internal package not allowed
internal/validation/validation.go:14:2: use of internal package not allowed
introspection/introspection.go:6:2: use of internal package not allowed
introspection/introspection.go:7:2: use of internal package not allowed

What happened?

This change will break compatibility and many people will have to rewrite their code. I don't want a repeat of the story > https://github.com/satori/go.uuid/issues/66

@sanae10001 are you still working within $GOPATH/src/github.com/neelance/graphql-go? You probably want to update your working directory.

@GhostRussia there may be some way that we can provide the new functionality without breaking clients (i.e. via some option or some temporary cleverness in the resolver lookup code).

Either way, we'll want to ensure we communicate the changes and impact of the changes before merging this in.

I think can use the interface to determine the type of resolver.

type Queryer interface {
	Query() interface{}
}

type Mutationer interface {
	Mutation() interface{}
}

And when using interfaces, we do not have to do extra reflection.

rFunc := root.MethodByName("Query")
if !rFunc.IsValid() {
	return nil, errors.New("not found query resolver")
}
 
queryResolver = rFunc.Call(nil)[0]

use:

type RootResolver struct {
}

func (r *Resolver) Query() interface{} {
	return &QueryResolver{}
}

func (r *Resolver) Mutation() interface{} {
	return &MutationResolver{}
}

graphql.MustParseSchema(schema, &RootResolver),

I checked here, this code allows working with two versions of resolvers. Original tests pass, but other tests are needed.

full resolvers patch: https://gist.github.com/GhostRussia/faa89c113cfca55366ee3867193d4d09#file-0001-separate-resolver-patch

Adapted tests patch: https://gist.github.com/GhostRussia/faa89c113cfca55366ee3867193d4d09#file-0002-separate-resolver-test-patch

and pass test https://github.com/graph-gophers/graphql-go/pull/182/files#diff-3a3269459d813f994deb0997a653aac2 after change

- func (r *RootResolver) Query() *QueryResolver {
+ func (r *RootResolver) Query() interface{} {
	return &QueryResolver{}
}

- func (r *RootResolver) Mutation() *MutationResolver {
+ func (r *RootResolver) Mutation() interface{} {
	return &MutationResolver{}
}

https://github.com/graph-gophers/graphql-go/pull/182#issuecomment-373405589 @tonyghita SemaphoreCI still working within $GOPATH/src/github.com/neelance/graphql-go for PR

Looks like we had two webhooks to the SemaphoreCI integration that were racing each other. I removed the old one that pointed to the previous repository, so we shouldn't see these issues.

Hi there,

Is there any ETA regarding this PR? How do you want to handle backward compatibilities?

Thanks a lot for your contribution.

@mytototo Now, it don't break compatibility. All above codes can correctly run. Maybe more tests are needed.

@sanae10001 Ok, thanks a lot.

Any plan on merging this?

Do you need any help with this PR to get it merged? @pavelnikolov

I'm trying to respond promptly to all new issues and PRs. I'm sorry for the delay on existing PRs. I'll review shortly.

I wonder if this can be done in a way that doesn't introduce a breaking change 🤔

I wonder if this can be done in a way that doesn't introduce a breaking change 🤔

It don't break compatibility. All above codes can correctly run.

BTW, what about implementing Go modules? This way we can introduce breaking changes with time when a gain is found.

On Fri, Nov 16, 2018, 6:07 AM Yingjie [email protected] wrote:

I wonder if this can be done in a way that doesn't introduce a breaking change 🤔 It don't break compatibility. All above codes can correctly run.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/graph-gophers/graphql-go/pull/182#issuecomment-439267185, or mute the thread https://github.com/notifications/unsubscribe-auth/AH9dxrjxYgI2R__A584-IDJ5tOO0i1XNks5uvivZgaJpZM4SroNG .

I wonder if this can be done in a way that doesn't introduce a breaking change 🤔

It don't break compatibility. All above codes can correctly run.

This is not completely true. You had to change the Star Wars example, right? This means that all users of the library will have to do the same when they switch to the latest version. It's not optional. It forces the users to upgrade.

BTW, what about implementing Go modules? This way we can introduce breaking changes with time when a gain is found.

Maybe it's a good idea. This in turn, would also increase complexity. I don't want to introduce something without a clear problem that it solves. This is a very dangerous path that can lead to a lot of unnecessary complexity with very little ROI.

@pavelnikolov Sorry, my mistake. The changes of Star Wars example only are legacy codes on commit https://github.com/graph-gophers/graphql-go/pull/182/commits/80ff5e1cd38c344cb5d706fe5ac7f2fec0e0dbef. Already restore.

@pavelnikolov I'm wondering what the state of this ticket is. As @sanae10001 mentioned, the PR has code to determine if the resolvers are separated or not, and falls back on the existing behavior.

@JohnMaguire it is not currently being worked on, as far as I know.

See https://github.com/graph-gophers/graphql-go/pull/421 for a workaround for that, we use this PR in production code, all our resolver's name are prefixed with either Query, Mutation or Subscription.

Excuse my ignorance @pavelnikolov but my impression was that this PR was ready for review but has since generated merge conflicts. Is there a reason it wasn't merged in 2018? Would it be merged if it were cleaned up today?

It's worth noting that the gqlgen feature comparsion page points to this issue as a point of distinction for their project.

@sanae10001 I started working on this and will try to add it to the next release. I like your code and I will copy some portion of it.

Closing this in favor of #561