-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Remove wrong Input Object coercion example #311
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
Conversation
The second line doesn't work: the types are wrong and it will produce an error. (See https://launchpad.graphql.com/z9ql4jq47)
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.
Thanks @andimarek! One minor request inline.
@@ -837,7 +837,6 @@ input ExampleInputObject { | |||
Original Value | Variables | Coerced Value | |||
----------------------- | --------------- | ----------------------------------- | |||
`{ a: "abc", b: 123 }` | {null} | `{ a: "abc", b: 123 }` | |||
`{ a: 123, b: "123" }` | {null} | `{ a: "123", b: 123 }` |
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.
Instead of removing it, can you update it to show that this produces an Error
?
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.
well .... the error is an error saying that the type is wrong. I don't think this has anything to do with input coercion. That's the reason I decided to remove 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.
I think that distinction is a little too subtle. Whether coercion fails because you didn't provide a required value, or because you provided one of the wrong type, the user is going to see an error, and probably will care more about the fact that it didn't work than whether it is technically a "coercion" error. Also, note that even though "coercion" is narrowly defined in the spec, it also has a more general meaning of translation from one type to another which many readers will have in mind when they look at this table (and might also explain why this example was here in the first place, even if it was wrong).
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 think the fundamental problem here is that this section Input coercion
mixes both aspects: "Input as ast literal" and "Input as variables (maps)".
For example in the first paragraph it states for the "Input as variables" aspect:
This unordered map should not contain any entries with names not defined by a field of this input object type, otherwise an error should be thrown.
But it doesn't say anything about extra fields for literals, because this is handled in the validation section. (I assume that is the reason)
In the table it is unclear if the "Original value" is a literal or a Map or both:
For example { b: $var }
can only be a literal, but the third entry { a: "abc" }
could be both.
And the second entry {a: 123, b: "123" }
is wrong if it is a literal, because the types are wrong.
BUT: in the reference implementation it is actually correct if {a: 123, b: "123" }
is interpreted as variable input. Could that be the reason it is there? (But I am not sure the reference implementation is correct in this case)
The second line doesn't work: the types are wrong and it will produce an error. (See https://launchpad.graphql.com/z9ql4jq47)