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

Adding GraphQL Extensions #184

Merged
merged 9 commits into from
Mar 30, 2022
Merged

Adding GraphQL Extensions #184

merged 9 commits into from
Mar 30, 2022

Conversation

janboll
Copy link
Contributor

@janboll janboll commented Mar 29, 2022

This enables accessing the Extensions field, as defined in the response
format: https://spec.graphql.org/October2021/#sec-Response-Format

Extensions can be enabled using configuration option use_extensions.
This will change the return parameters of the generated client
functions. Making it a breaking change if enabled.

Since extensions are untyped as defined in the spec, the Client will
return an interface of type map[string]interface{}.

Issue: #183

I have:

  • Written a clear PR title and description (above)
  • Signed the Khan Academy CLA
  • Added tests covering my changes, if applicable
  • Included a link to the issue fixed, if applicable
  • Included documentation, for new features
  • Added an entry to the changelog

This enables accessing the Extensions field, as defined in the response
format: https://spec.graphql.org/October2021/#sec-Response-Format

Extensions can be enabled using configuration option use_extensions.
This will change the return parameters of the generated client
functions. Making it a breaking change if enabled.

Since extensions are untyped as defined in the spec, the Client will
return an interface of type map[string]interface{}.
Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for writing it and for the clear commit message and documentation.

I have a couple of small comments inline; the most important thing is the changes in the API surface graphql/client.go, where we just want to be a little picky to get everything right before it's a breaking change. Everything else is just a suggestion.

Then the last thing is if you could add a changelog entry (under the "vNext" section at the top) -- or actually two; one for the new feature and one for the breaking change to the MakeRequest interface (you can crib from the similar one in v0.2.0).

generate/operation.go.tmpl Outdated Show resolved Hide resolved
generate/operation.go.tmpl Outdated Show resolved Hide resolved
generate/testdata/queries/foo.go Outdated Show resolved Hide resolved
graphql/client.go Outdated Show resolved Hide resolved
graphql/client.go Outdated Show resolved Hide resolved
graphql/client.go Show resolved Hide resolved
@benjaminjkraft
Copy link
Collaborator

Great, thanks for the changes and for the contribution! I put in a few tiny tweaks to the documentation, which reminded me that this also fixes #19.

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.

2 participants