Do not include unhelpful scalar types.

[dmitshur]

This is an issue with the current API design I've come to feel by using the API more, by thinking about it more, and through some user feedback (/cc @slimsag).

Right now, githubql defines a custom type for each GitHub GraphQL API scalar:

https://developer.github.com/v4/reference/scalar/

https://github.com/shurcooL/githubql/blob/bd2ca14c68143d6449401ed5c65ddefe7ef9058f/scalar.go#L19-L61

Some of them are very primitive types that the Go type system already offers as built in types:

// Boolean represents true or false values.
type Boolean bool

// Float represents signed double-precision fractional values as
// specified by IEEE 754.
type Float float64

// Int represents non-fractional signed whole numeric values.
// Int can represent values between -(2^31) and 2^31 - 1.
type Int int32

// String represents textual data as UTF-8 character sequences.
// This type is most often used by GraphQL to represent free-form
// human-readable text.
type String string

I created scalar.go completely by hand (it's so small, and unlikely to change often, that auto-generating it from schema wasn't worth it at this time). The thinking was that githubql should expose all types in the GitHub GraphQL schema, and scalars were a small and easy place to start. So I added all of them. Which lets you write code like this:

var query struct {
	Viewer struct {
		Login     githubql.String
		CreatedAt githubql.DateTime
	}
}

Some early user feedback immediately pointed it out as a source of uncertainty/unexpected complexity:

I'm sure I could figure out the reason why from looking at the code, but I wonder if the githubql.String type is really needed? I wonder why string can't be directly used, especially since variables := map[string]interface{} has a value interface{} type meaning forgetting to wrap it in a githubql.String can introduce subtle bugs not caught by the compiler

And the answer is those custom types are absolutely not needed. In fact, I had a note at the top of schalar.go:

// Note: These custom types are meant to be used in queries, but it's not required.
// They're here for convenience and documentation. If you use the base Go types,
// things will still work.
//
// TODO: In Go 1.9, consider using type aliases instead (for extra simplicity
//       and convenience).

After writing more and more code that uses githubql, I've come to realize these types are simply counter-productive. Using them adds no value to the code, only makes it more verbose by forcing you to do type conversions. Not using them feels weird because they exist, and README demonstrates them being used.

Compare, using them:

variables := map[string]interface{}{
	"repositoryOwner": githubql.String(repo.Owner),
	"repositoryName":  githubql.String(repo.Repo),
	"issueNumber":     githubql.Int(id),
	"timelineCursor":  (*githubql.String)(nil),
	"timelineLength":  githubql.Int(100),
}
if opt != nil { // Paginate. Use opt.Start and opt.Length.
	if opt.Start != 0 {
		variables["timelineCursor"] = githubql.NewString(githubql.String(cursor))
	}
	variables["timelineLength"] = githubql.Int(opt.Length)
}

Vs using Go's equivalent built in types:

variables := map[string]interface{}{
	"repositoryOwner": repo.Owner,
	"repositoryName":  repo.Repo,
	"issueNumber":     id,
	"timelineCursor":  (*string)(nil),
	"timelineLength":  100,
}
if opt != nil { // Paginate. Use opt.Start and opt.Length.
	if opt.Start != 0 {
		variables["timelineCursor"] = githubql.NewString(cursor)
	}
	variables["timelineLength"] = opt.Length
}

Additionally, sometimes you query for an int:

Nodes []struct {
	Number githubql.Int

And immediately try to convert it to uint64, for example:

Number: uint64(issue.Number),

But instead, you might as well ask for a uint64 to be decoded into in the query:

Nodes []struct {
	Number uint64

Number: issue.Number,

This should not be more unsafe. As long as a query is executed successfully at least once, it'll always work. Besides, someone could've accidentally used githubql.String where githubql.Int should've been used, the compiler wouldn't catch that either.

The only useful purpose they serve is documentation of GitHub GraphQL scalars, but I think we need to provide such documentation without actually having the unneeded types.

[dmitshur]

There's a really good way analogy I can think of that shows how silly it is to have these types. Imagine if the encoding/json package defined types such as:

package json

// Boolean represents JSON booleans.
type Boolean bool

// Float represents JSON numbers.
type Float float64

// String represents JSON strings.
type String string

And suggested that users did this:

var jsonBlob = []byte(`[
	{"Name": "Platypus", "Order": "Monotremata"},
	{"Name": "Quoll",    "Order": "Dasyuromorphia"}
]`)
type Animal struct {
    Name  json.String
    Order json.String
}
var animals []Animal
err := json.Unmarshal(jsonBlob, &animals)
if err != nil {
    fmt.Println("error:", err)
}
fmt.Printf("%+v", animals)

[dmitshur]

That said, we don't want to remove all scalar types. Only those that are easily replaceable by built in Go types.

For example, we'll remove githubql.DateTime, because time.Time can be used in its place.

But we'll keep githubql.URI, because *url.URL cannot be used in its place. *url.URL does not implement json.Unmarshaler interface, but githubql.URI does.

[dmitshur]

I tried implementing this, and quickly found out that removing the primitive scalar types isn't viable.

There are two directions to consider when it comes to interacting with a GraphQL API. One is sending data from your program (i.e., inputs, variables), and the other is receiving the results (query response).

When dealing with the response, we can consider making use of encoding/json flexibility and unmarshal the values from the response (which is JSON marshaled) into types like int, string, etc.

However, when it comes to sending data to GraphQL API, the only types it supports are those primitives that are defined. GraphQL will only accept an Int if that's the documented type, it can't know of int or int32.

I'll keep this open and keep thinking/experimenting with what can be done to improve the user experience on the receiving response side. But all scalars will stay, because they're needed when sending input.

[gcrevell]

Not sure this is the best place for this, but is there a way to convert the githubql.String to native string?

[dmitshur]

@gcrevell You can use a conversion, e.g.:

var s1 githubql.String
var s2 string
s2 = string(s1) // type conversion

If string is what you want, you can just use it directly in your query instead of githubql.String. E.g.:

var query struct {
	Viewer struct {
		Login string
	}
}

(You only really need to use githubql.String for arguments and variables.)

I want to update the README to make this information more accessible.

[mark-rushakoff]

In hopes of helping anyone else who might run into this:

I had a query that looked like:

// PullRequestHeadBranch gets the head branch and node ID of a given pull request.
func (c *V4Client) PullRequestHeadBranch(ctx context.Context, repo Repo, prNumber int) (branchName, nodeID string, err error) {
	var q struct {
		Repository struct {
			PullRequest struct {
				ID          string
				HeadRefName string
			} `graphql:"pullRequest(number: $prNumber)"`
		} `graphql:"repository(owner: $owner, name: $name)"`
	}
	vars := map[string]interface{}{
		"owner":    repo.Owner, // <-- Problem! Don't use a plain string.
		"name":     repo.Name,// <-- Problem! Don't use a plain string.
		"prNumber": prNumber, // <-- Problem! Don't use a plain int.
	}

	if err := c.Client.Query(ctx, &q, vars); err != nil {
		return "", "", err
	}

	pr := q.Repository.PullRequest
	return pr.HeadRefName, pr.ID, nil
}

And I was getting an error int isn't a defined input type (on $prNumber) returned from GitHub when executing it. (There's only a couple web search results for isn't a defined input type, so I hope this comment makes it a little more discoverable.)

Changing the lines with the Problem! comments to wrap the corresponding values in githubv4.ID and githubv4.Int fixed the query and made it start working as intended.

[davidkarlsen]

@dmitshur how can I set a query-variable to null? If I have a map[string]interface and set "somekey": nil it fails at writeArgumentType. I have a pagingElement which should be null on 1st page.

[dmitshur]

@davidkarlsen Something like this should work:

variables := map[string]interface{}{
	"somekey": (*githubv4.String)(nil), // or another concrete type instead of String
	// ...
}

[davidkarlsen]

@davidkarlsen Something like this should work:

variables := map[string]interface{}{
	"somekey": (*githubv4.String)(nil), // or another concrete type instead of String
	// ...
}

Thanks - I found info in another issue too - it works fine now.

[alexellis]

Is it possible to pass an array for an argument, like in the GraphQL example here? https://gist.github.com/alexellis/6212c988189323dbb2806d1c7f7699ab