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

Conform built in types to spec #1017

Conversation

maartenvanvliet
Copy link
Contributor

Changes the built in types to conform to the spec, see https://spec.graphql.org/draft/#sec-Schema-Introspection
Most notably adds the __TypeKind enum.

Absinthe.Introspection.Kind.kind now returns an atom for the __TypeKind enum resolver.

See https://spec.graphql.org/draft/#sec-Schema-Introspection

`Absinthe.Introspection.Kind.kind` now returns an atom. It wasn't
called anywhere except from introspection but it is a breaking change.
Copy link
Contributor

@binaryseed binaryseed left a comment

Choose a reason for hiding this comment

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

Thanks, this is a good improvement!

@@ -8,19 +8,20 @@ defmodule Absinthe.Introspection.Kind do
__MODULE__
|> Module.split()
|> List.last()
|> Absinthe.Introspection.Kind.upcase()
|> Absinthe.Introspection.Kind.downcase()
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this is enumerated now, and not just a string, I wonder if this can be simplified (ie: not use all that module / string manipulation)

It seems like the goal is just to have a function kind that returns an atom... We could get rid of the macro altogether:

defmodule Absinthe.Type.Object do
  @behaviour Absinthe.Introspection.Kind
  def kind, do: :object
end

defmodule Absinthe.Introspection.Kind do
  @type introspection_kind :: :enum | :object # ...
  @callback kind() :: introspection_kind()
end

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'll do this separately...

Copy link
Contributor

@binaryseed binaryseed Jan 2, 2021

Choose a reason for hiding this comment

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

Actually while making this change, I found some further issues with our __TypeKind!

Further corrections here: #1019

+1 to being explicit!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thx, good catch!

@binaryseed binaryseed merged commit 4a2573e into absinthe-graphql:master Jan 2, 2021
@maartenvanvliet maartenvanvliet deleted the issues/conform-introspection-schema-spec branch September 17, 2021 11:58
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.

None yet

2 participants