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

[2021-03-04] Implement Oneof Input Objects in GraphQL.js #648

Closed
benjie opened this issue Mar 7, 2021 · 12 comments · Fixed by graphql/graphql-js#3513
Closed

[2021-03-04] Implement Oneof Input Objects in GraphQL.js #648

benjie opened this issue Mar 7, 2021 · 12 comments · Fixed by graphql/graphql-js#3513
Assignees
Labels
Action item 🎬 Ready for review 🙌 Action Item issues are reviewed and closed during Working Group meetings.

Comments

@benjie
Copy link
Member

benjie commented Mar 7, 2021

RFC PR

ACTION (Benjie) - Write an implementation in GraphQL.js


Note: Action Item issues are reviewed and closed during Working Group
meetings.

@benjie benjie self-assigned this Mar 7, 2021
@benjie benjie added this to To do in Actions 2021-03-04 via automation Mar 7, 2021
@erikkessler1
Copy link

Hey @benjie, I've been following your Oneof RFC and some of the discussions around it in the GraphQL Working Group. I'm excited about the proposal and want to help move the RFC forward. I have experience with GraphQL and TypeScript, so I'd be interested in helping write an implementation for GraphQL.js. Did you already have an implementation in progress or should I just start from scratch? Also, from this this comment, it sounds like you're still working through the syntax—do you have any thoughts on which syntax would be best to implement? Please let me know how I can help!

@benjie
Copy link
Member Author

benjie commented Jan 4, 2022

Hey @erikkessler1; that's great! There's a few open questions:

  1. Which things will we support: input object types, object types, field arguments?
  2. What's the syntax?

I think it's fine to drop field arguments ("Oneof Fields" in the spec edit) and go with just object types and input object types. I think it's also wise to simply use the directive syntax for now - at least then we can play with it :) There's quite a bit of pushback on using @OneOf with output types (due to interfaces and unions already existing) but I'd like to see it implemented and then we can turn it off if we must to get it merged.

I don't have an in progress PR currently, I was planning to start work on it in February/March, got a few other things to clear off my plate first.

@erikkessler1
Copy link

Starting with input object types and object types using the directive syntax makes sense to me. I'll see what I can do working off of those assumptions!

@erikkessler1
Copy link

Hey @benjie, I've got some initial progress to share that I'd love to get your early feedback on. For consumability, I've broken my implementation into a few PRs on my fork of graphql-js:

(The combined diff is here)

Let me know your thoughts or what next steps I should take.

@benjie
Copy link
Member Author

benjie commented Jan 28, 2022

Excellent work; I didn't see anything in the changes that didn't align with my intent. I've not had time to fully review (I need to get back to doing the spec edits at some point!) but it's looking good to me 👍

@erikkessler1
Copy link

Great, thank you for taking a look! Are there any other immediate action items you can think of for me?

@benjie
Copy link
Member Author

benjie commented Jan 28, 2022

Nope; just try it out in a real schema and see how you get on! 🙌

@nazreen
Copy link

nazreen commented Mar 8, 2022

any updates on this? :) we have a use case that could benefit from this solution

@benjie
Copy link
Member Author

benjie commented Mar 22, 2022

The RFC has been updated to drop Oneof Fields, I still need to find some time to test @erikkessler1's work above though. Have you tried it @nazreen?

@michaelstaib
Copy link
Member

So oneof arguments is still included? For Hot Chocolate we implemented oneof inputs but did not implement the arguments yet. I personally see value in having oneof arguments. What is the current sentiment around these?

@benjie
Copy link
Member Author

benjie commented Mar 25, 2022

“Oneof fields” are fields that accept one argument. The general feeling is that the goal is easy to achieve by using a oneof input object as the type of the only argument, and that not doing this limits schema evolution options. I’ve thus removed them.

The main reason to consider them is syntax; where does the directive/keyword/etc go?

@benjie benjie added the Ready for review 🙌 Action Item issues are reviewed and closed during Working Group meetings. label May 5, 2022
@leebyron leebyron closed this as completed May 5, 2022
Actions 2021-03-04 automation moved this from To do to Done May 5, 2022
@benjie
Copy link
Member Author

benjie commented May 6, 2022

Thanks to your work @erikkessler1, we've moved @oneOf to RFC2! 🙌

IvanGoncharov pushed a commit to graphql/graphql-js that referenced this issue Jun 27, 2023
Co-authored-by: Benjie Gillam <benjie@jemjie.com>
Closes graphql/graphql-wg#648
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Action item 🎬 Ready for review 🙌 Action Item issues are reviewed and closed during Working Group meetings.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants