Pass call type in from/toGraphQL #1257
Conversation
Currently types are ignored and not getting passed in `callsFromGraphQL` or `callsToGraphQL`. This results in dangerous output in `toGraphQL`, in which all calls have type null. This can cause problems when using non-scalar types (like enum). Example can be found [here](facebook#1256).
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
ghost
added
the
| @@ -42,7 +43,11 @@ function callsFromGraphQL( | |||
| const orderedCalls = []; | |||
| for (let ii = 0; ii < callsOrDirectives.length; ii++) { | |||
| const callOrDirective = callsOrDirectives[ii]; | |||
| let {value} = callOrDirective; | |||
| let {value, metadata} = callOrDirective; | |||
| let type = undefined; | |||
|
@papigers Thanks for the PR! Be sure to run the tests locally; that's what is failing in the build. Some tests look at the actual shape of the "concrete" query which will be different after this PR, so those tests will need to be updated. If there's a test failure that isn't as obvious, feel free to comment here and we can help. |
|
@josephsavona Will do, working on that right now, sorry for this noobish PR. Next time will be better, I hope ;) |
|
Sorry for the uninformatives commits |
| @@ -23,7 +23,7 @@ import type RelayQuery from 'RelayQuery'; | |||
|
|
|||
| export type Call = { | |||
| name: string, | |||
| type?: string, | |||
| type?: ?string, | |||
There was a problem hiding this comment.
I haven't thought about this as deeply as you two have, but might we instead want type to either be a string or not supplied?
const orderedCall = {
name: callOrDirective.name,
value,
};
if (metadata && metadata.type) {
orderedCall.type = metadata.type;
}
orderedCalls.push(orderedCall);That would cut down on the noise in the tests, among other things. If it's important to have type: null then, carry on!
There was a problem hiding this comment.
Yeah, this already is marked as an optional field.
@papigers I apologize for the back and forth on this. Since this is already an optional field, how about reverting the addition of all the type: nulls to focus this PR on just the logic & results that actually change?
| @@ -23,9 +23,14 @@ import type {Variables} from 'RelayTypes'; | |||
|
|
|||
| const invariant = require('invariant'); | |||
|
|
|||
| type Metadata = { | |||
| type: ?string, | |||
There was a problem hiding this comment.
let's make this type?: ?string
|
@papigers Thanks again for this PR. I commented to request some changes. Also, scanning through i don't see any tests that actually end up with I apologize if I missed something, but if not can you add a test that has a non-null type? This would be an enum or object argument. |
|
I just ran into the same issue - any updates on this? |
|
@papigers are you able to update the PR? |
|
Not any time soon , I'm afraid. Too busy.. |
Currently types are ignored and not getting passed in
callsFromGraphQLor
callsToGraphQL.This results in dangerous output from
toGraphQL, in which all calls havetype null. This can cause problems when using non-scalar types (like
enum).
For more details see #1256.