Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC: OneOf Input Objects #825
base: main
Are you sure you want to change the base?
RFC: OneOf Input Objects #825
Changes from 3 commits
c385058
f6bd659
39e593c
b6741c3
d17d5ec
dca3826
7e02f5a
4111476
6754e0a
7c4c1a2
bb225f7
05fde06
e8f6145
08abf49
59cb12d
c470afb
99aa5d9
691087d
7109dbc
05ab541
6a6be52
de87d2f
57e2388
5a966f2
e78d2b5
c6cd857
d106233
87d0b22
d88d62a
a810aef
a1563a9
b45c0e4
0c9830e
c4d0b50
340594e
dbccf84
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels kinda awkward, like we're skirting around the idea of having fundamentally different types of fields without really committing to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing
{ a: $varA, b: $varB }
with various combinations of values for varA and varB.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My in meeting proposal was that this case could just be invalid at start.
This L1441 in Validation file in this PR sounds like it would do just that:
https://github.com/graphql/graphql-spec/pull/825/files#diff-607ee7e6b71821eecadde7d92451b978e8a75e23d596150950799dc5f8afa43eR1441
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are exactly the same as for input objects (which also don't specify what happens if you have multiple variables); but I'll add some for clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leebyron Good catch; that was not my intent. I have updated the PR with better validation and more examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've since revisited my thoughts on this and for the sake of defining types of variables on the client I've adopted the suggestion: #825 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or
oneArg
to inline withargs
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the word literal appropriate here in case of using variables? The same question about Oneof for Input Object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe so; I've modeled it on the language already used in this section, namely: https://spec.graphql.org/draft/#sel-LALTHHDHFFFJDAAACDJ-3S