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

Add SDL/macro type extensions #1157

Merged

Conversation

maartenvanvliet
Copy link
Contributor

@maartenvanvliet maartenvanvliet commented Feb 16, 2022

Gave it a shot to add support for extend to SDL. Once that was running, adding macro support was not that difficult.
It follows the graphql spec for the most part, though schema declarations cannot be extended as of yet. The validations are done by the currently existing validation phases.

I looked at #821 but decided against the

extend :foo do
   field :name, :string
end

syntax in favor of

extend do 
   object :foo do
      field :name, :string
   end
end

since it was easier to implement and is closer to the graphql syntax. I also think it will work better with imports.

Imports across modules work as well, using the type_extensions field. It uses the TypeImports phase, though it could be moved to a different module if necessary or even a separate import_type_extensions macro instead of the import_types macro.

Fixes #772

@kdawgwilk
Copy link
Contributor

Could this support this syntax? Since that is what absinthe_relay uses and may already be familiar to consumers of absinthe https://hexdocs.pm/absinthe_relay/Absinthe.Relay.Node.html#module-object

extend object :foo do
    field :name, :string
end

absinthe_relay

node object :person do
  field :name, :string
  field :age, :string
end

@maartenvanvliet
Copy link
Contributor Author

Ah interesting, forgot about that syntax usage in relay. I'll have a look.

@maartenvanvliet
Copy link
Contributor Author

Thx! Fixed it in 675808a

@maartenvanvliet
Copy link
Contributor Author

The formatting failures in CI were pre-existing

@maartenvanvliet maartenvanvliet force-pushed the issues/type_extensions branch 2 times, most recently from e2a053c to cf372f9 Compare February 17, 2022 13:00
Copy link
Contributor

@benwilson512 benwilson512 left a comment

Choose a reason for hiding this comment

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

Excellent stuff, can you add a changelog entry?

@maartenvanvliet
Copy link
Contributor Author

Thx! I've added an entry 🎉

schema = %{
schema
| directive_definitions: directives,
type_extensions: type_extensions,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you still need this in this phase now that you added that other ApplyTypeExtensions phase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've extracted the type extension imports to a separate phase, like with import_directives.

@maartenvanvliet
Copy link
Contributor Author

Changed the import to use import_type_extensions analogous to import_types and import_directives.

Also, added a fix to extend builtin types through type extensions and changed the DeprecatedDirectiveFields phase to use that.

@maartenvanvliet
Copy link
Contributor Author

maartenvanvliet commented Feb 18, 2022

Moved the defaults to the pipeline configuration. This way the DeprecatedDirectiveFields phase is no longer needed as the same can be achieved with configuration.

@kdawgwilk sorry, this clashes a bit with your PR. The same structure can be applied there.

@benwilson512
Copy link
Contributor

@maartenvanvliet What is the overall state of this PR?

@maartenvanvliet
Copy link
Contributor Author

@maartenvanvliet What is the overall state of this PR?

Just fixed a merge conflict with the changelog. Now, it's good to go. 🚀

I have an idea around extending scalar types, so a type extension can be used to make the Int type spec compliant (see #921) It goes beyond the spec and can be left for another PR though.

@benwilson512 benwilson512 merged commit 658d510 into absinthe-graphql:master Apr 26, 2022
@maartenvanvliet maartenvanvliet deleted the issues/type_extensions branch March 19, 2023 18:22
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.

Support "extend" keyword
3 participants