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

Simplify graphql.Client interface (MakeRequest) #19

Closed
benjaminjkraft opened this issue Apr 22, 2021 · 2 comments
Closed

Simplify graphql.Client interface (MakeRequest) #19

benjaminjkraft opened this issue Apr 22, 2021 · 2 comments
Labels
needs design Issues where we don't know exactly what we need yet; discuss on the issue before implementing

Comments

@benjaminjkraft
Copy link
Collaborator

benjaminjkraft commented Apr 22, 2021

The graphql.Client interface is scarily complex, for something we have to support forever. We may want to try to make it a bit more constrained, with specific hooks for specific things, so that callers can't do weird things. But we'll need to see how people actually use it.

At Khan, we currently:

  • munge the URL to add the opname
  • munge the error (if any), wrapping, and potentially adding some of the variables

Other potential use cases:

@benjaminjkraft benjaminjkraft changed the title Simplify graphql.Client interface Simplify graphql.Client interface (MakeRequest) May 1, 2021
@benjaminjkraft
Copy link
Collaborator Author

Another option is to do

type Request struct {
  OperationName string
  Query string
  Variables map[string]interface{}
}
type Client interface {
  MakeRequest(ctx context.Context, request *Request, retval interface{})
}

and then we could at least add fields.

We could even replace retval interface{} with returning the json.RawMessage, or with passing you unmarshal(string) interface{} and having you return the interface{}, although both are a bit ugly and I'm not sure either is quite flexible enough.

@benjaminjkraft benjaminjkraft added the needs design Issues where we don't know exactly what we need yet; discuss on the issue before implementing label Sep 11, 2021
@benjaminjkraft
Copy link
Collaborator Author

A great solution similar to my above comment fell out in #184, so I think we are all set here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs design Issues where we don't know exactly what we need yet; discuss on the issue before implementing
Projects
None yet
Development

No branches or pull requests

1 participant