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 InputUnion problem statement #618

Merged
merged 7 commits into from
Sep 19, 2019

Conversation

binaryseed
Copy link
Collaborator

This PR adds a section to the InputUnion RFC document that provides an overview of the problem being addressed.

Open to feedback!

@IvanGoncharov @leebyron @benjie

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

This is great!

I've tried to take on the role of editor and added suggestions to make the text slightly more formal, and have replaced instances of APIs with GraphQL schema as appropriate to be more explicit/accurate. I've softened the language in a few places (and strengthened it in others).

One thing I'm concerened with is that it seems to focus on mutations, but using input unions in queries is also a desired outcome of this solution. Accommodating that in the text would be wonderful. I'm not sure how to do this concisely.

Once issue that I think needs addressing is the reference to Dog, Cat etc as the input types, whilst also talking about how the input types should mirror the output types. If Dog and Cat are the input types, what are the output types called? I suggest renaming all input type references to be *Input throughout, for example AnimalInput, CatInput, etc. This is particularly important with the final __typename reference to "Dog" which seems like it's pointing at an output type.

Let me know what you think, hopefully this feedback is helpful 😄

rfcs/InputUnion.md Outdated Show resolved Hide resolved
rfcs/InputUnion.md Outdated Show resolved Hide resolved
rfcs/InputUnion.md Outdated Show resolved Hide resolved
rfcs/InputUnion.md Outdated Show resolved Hide resolved
rfcs/InputUnion.md Outdated Show resolved Hide resolved
rfcs/InputUnion.md Outdated Show resolved Hide resolved
rfcs/InputUnion.md Outdated Show resolved Hide resolved
rfcs/InputUnion.md Show resolved Hide resolved
@IvanGoncharov
Copy link
Member

IvanGoncharov commented Sep 16, 2019

@binaryseed Thanks for keeping the ball rolling 👍
But I don't agree that the core problem here is that:

GraphQL currently provides abstract types that operate on Object types, but they aren't allowed on Input types.

GraphQL has two distinct type systems by design: nominal (for output type) and structural (for input types). But the current problem statement looks like core issue here is to switch input types to nominal and support abstract types. That definitely can be one of the solutions but it's not the only one, e.g. @benjie @oneField proposal doesn't add any abstract type and keeps input types nominal.

I think we need to describe what we want to achieve and not how we want to do that.
So I think core problem here is the ability to describe complex input types that can have multiple shapes (e.g. sets of fields) without losing type-safety so instead of abstract type we can use polymorphic type.

I also think the problem statement should be short and mutation example (with tag union workaround) should be part of use cases section because right now it looks like it's the only problem we trying to solve.

rfcs/InputUnion.md Outdated Show resolved Hide resolved
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Fantastic work @binaryseed 💯 🙌

@IvanGoncharov IvanGoncharov merged commit baa73cd into graphql:master Sep 19, 2019
@IvanGoncharov
Copy link
Member

Merged 🎉
@binaryseed Thanks a lot for keeping the ball rolling 👍
@benjie @jturkel Thanks for taking the time to review 👍

@binaryseed binaryseed deleted the input-union-problem branch September 19, 2019 22:56
@leebyron leebyron added the 📣 RFC document PR creates or changes document inside "rfc" folder label Apr 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📣 RFC document PR creates or changes document inside "rfc" folder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants