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 extend to macro defined schemas #821

Closed

Conversation

binaryseed
Copy link
Contributor

@binaryseed binaryseed commented Dec 9, 2019

This PR introduces an extend macro that behaves as the "extend" concept in the GraphQL spec:

We can extend object, input_object, enum, union, and interface.

The GraphQL spec allows extending schema and scalar but that isn't supported here - there isn't a schema macro to extend and scalars can only be extended to have more directives which also isn't supported in macro schemas

For now this works with macro based schemas, but it should be possible to adapt this to SDL based schemas as well (see #772)

@benwilson512
Copy link
Contributor

Your strategy so far looks sound, is there any particular part you'd like feedback on?

@binaryseed
Copy link
Contributor Author

At first I wasn't sure this would work but I can see it coming together now, I'll let you know if anything arises!

@binaryseed
Copy link
Contributor Author

So this works for straight forward things like object, input_object, enum just fine

It's a little tricky with union - I got it to merge types which is cool but it seems pretty complicated to handle stuff like resolve_type... I guess in that situation people could use is_type_of

The spec lets you extend just about everything via SDL but we probably don't have to go quite that far to get a lot of value...

@binaryseed binaryseed marked this pull request as ready for review January 2, 2020 07:22
@binaryseed binaryseed changed the title Experiment: support for extend Add extend to .macro defined schemas Jan 2, 2020
@binaryseed binaryseed changed the title Add extend to .macro defined schemas Add extend to macro defined schemas Jan 2, 2020
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.

@binaryseed I need to review in more detail, but from a high level is the actual extension happening in build_types? This approach won't be able to support extending types from other modules, since the attribute stack => blueprint transform is local to a given module.

My sense of how extend would work is that we'd probably create explicit extension blueprint extension structs that would sit under blueprint.extensions. Then there would be a phase in the schema builder pipeline that would apply extensions to the relevant type. Errors like "doesn't exist" could be placed on the extensions as ordinary errors and would fall into the normal error handling we have for schema validity.

@@ -28,8 +28,7 @@ defmodule Absinthe.Schema.Notation do
Module.register_attribute(__CALLER__.module, :absinthe_blueprint, accumulate: true)
Module.register_attribute(__CALLER__.module, :absinthe_desc, accumulate: true)
put_attr(__CALLER__.module, %Absinthe.Blueprint{schema: __CALLER__.module})
Module.put_attribute(__CALLER__.module, :absinthe_scope_stack, [:schema])
Module.put_attribute(__CALLER__.module, :absinthe_scope_stack_stash, [])
put_scope_stack(__CALLER__.module, [:schema])
Copy link
Contributor

Choose a reason for hiding this comment

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

@binaryseed In master I added https://github.com/absinthe-graphql/absinthe/blob/master/lib/absinthe/schema/notation.ex#L1508 to deal with the fact that we needed two stacks to handle stack stashing for Absinthe Relay, we should unify these helpers.

@GTDev87
Copy link

GTDev87 commented Jun 10, 2020

Any progress on this issue? It would be very useful for a dataloader pattern I'm using in my codebase.

@Leeaandrob
Copy link

Any news about the issue?

@benwilson512
Copy link
Contributor

This has since been implemented in a different PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants