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

allow unions to include interfaces and unions #950

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented May 27, 2022

Complements #939
Addresses #711

Similar to #939, this PR expands the robustness of the type system by allowing types that actually fulfill interfaces to be recognized as such by the GraphQL type system.

interface CloningInfo {
  ...
}

union CowOrWolf implements Animal = Cow | Wolf  # allowed by #939
union CowOrCloningInfo = Cow | CloningInfo  # allowed by this PR, note that CloningInfo is an interface
union WolfOrCloningInfo = Wolf | CloningInfo # allowed by this PR, note that CloningInfo is an interface

# note that here we are marking unions explicitly as included within a union.
# Adding a type to the CowOrWolf union will automatically add it to the ParentUnion
# We could also consider adding a constraint on the union definition, see below discussion
union Parent = CowOrCloningInfo | WolfOrCloningInfo | CowOrWolf | Cow | Wolf | CloningInfo

interface Animal {
  parent: Parent
}

type Cow implements Animal {
    parent: CowOrCloningInfo  # unlocked by this PR
}

type Wolf implements Animal{
    parent: WolfOrCloningInfo # unlocked by this PR
} 

Unions

With regard to unions, the goal is to explicitly mark some unions as members of other unions. We have two alternatives:

(A). Let unions include unions as members, as shown above. We could (or could not) require that all members of the unions also be listed (similar to how interfaces implementing child interfaces are required to explicitly list the parent.

Pro:

  1. Simple to reason about, fits with how unions currently work.
    Con:
  2. Could lead to some automatic behavior, when adding a type to a union that is part of a union, the type gets added to multiple unions. This is ameliorated by requiring all child union members to be explicitly listed.
  3. Union definitions could start to get pretty long if all the combinations must be listed therein.

(B) Add an additional optional constraint on the union requiring all of the members to be members of some other union, similar to how we have resolved #939.

Potential Syntax:

union CowOrWolf implements Animal, subtypes Parent = Cow | Wolf 
union CowOrCloningInfo subtypes Parent = Cow | CloningInfo 
union WolfOrCloningInfo subtypes Parent = Wolf | CloningInfo

union Parent = Cow | Wolf | CloningInfo
# cf
# union Parent = CowOrCloningInfo | WolfOrCloningInfo | CowOrWolf | Cow | Wolf | CloningInfo

Interfaces

For interfaces that are members of unions, it would not seem to make sense to require all the implementations of the interfaces to be listed independently. The whole point is that it is often just as useful to indicate that several interfaces might be returned as it is that several individual member types might be returned. For unions, we also have potentially multiple layers of nesting (unions of unions of unions) for which it would be extremely helpful to require the individual member types to be listed (or to use the second syntax above) while we don't have the same issue with interfaces.

@netlify
Copy link

netlify bot commented May 27, 2022

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit d8e52f0
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/62e200650ec7d60008f9f9d5
😎 Deploy Preview https://deploy-preview-950--graphql-spec-draft.netlify.app/draft
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@netlify
Copy link

netlify bot commented May 27, 2022

Deploy Preview for graphql-spec-draft failed.

Name Link
🔨 Latest commit 18e70e5
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/62907cf3e15503000982fb5a

@yaacovCR
Copy link
Contributor Author

Now implemented in graphql/graphql-js#3682

I added requirement that all subtypes of included abstract types must be explicitly includes within the union. This can always be relaxed later.

with regard to requirement to list all transitive possible types, including member interfaces
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💡 Proposal (RFC 1) RFC Stage 1 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants