Add more information in conversion error#1579
Conversation
|
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. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
The pipeline failed but I'm not sure why. TBH it's the first time I do a PR for a flow project (I'm usually using Typescript). |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
src/execution/execute.js
Outdated
| }.`, | ||
| }. Value ${inspect(result)} cannot be converted to ${inspect( | ||
| returnType, | ||
| )}`, |
There was a problem hiding this comment.
You can use + to produce better formatting:
`Cannot return null for non-nullable field ` +
`${info.parentType.name}.${info.fieldName}. ` +
`Value ${inspect(result)} cannot be converted to ${inspect(returnType)}`
There was a problem hiding this comment.
I also think converted is not a correct term here, the spec calls it coercion:
https://facebook.github.io/graphql/draft/#example-cb7e7
Also, see 4a here:
https://facebook.github.io/graphql/draft/#CompleteValue()
There was a problem hiding this comment.
Cannot return null for non-nullable field DataType.test. Value null cannot be converted to [Int!]! is strange error.
You can do:
if (result === null) {
throw new Error(`Cannot return null for non-nullable field ${info.parentType.name}.${info.fieldName}.`);
} else {
throw new Error(`<Another error specifically about coercion>`);
}There was a problem hiding this comment.
Thanks for the advice, I'll do that.
There was a problem hiding this comment.
How does this look?
if (result === null) {
throw new Error(
`Cannot return null for non-nullable field ` +
`${info.parentType.name}.${info.fieldName}.`,
);
} else {
throw new Error(
`Cannot coerce '${inspect(result)}' into type ` +
`${inspect(returnType)} for non-nullable field ` +
`${info.parentType.name}.${info.fieldName}`,
);
}There was a problem hiding this comment.
@domi7777 I still not sure about couple edge cases that could mismatch with this error message. I need to refresh my knowledge of this code to be sure so I will try to do that on weekend.
There was a problem hiding this comment.
@IvanGoncharov - mind sharing some edge cases you can imagine?
There was a problem hiding this comment.
@Neitsch @domi7777 Hi sorry for the radio silence.
I have some project that I need to finish.
I will try to review and merge all stale PRs in the next couple of weeks.
mind sharing some edge cases you can imagine?
To be honest I already forgot. I will try to find some time next week to properly review it. Also feel free to ping me.
Add more information in conversion error: fix pipeline
mjmahone
left a comment
There was a problem hiding this comment.
This seems like a positive improvement from my end.
I've often had this error:
Cannot return null for non-nullable field x.I find it easier to understand why the error occurred with a bit more of information, eg:
Cannot return null for non-nullable field Transition.date. Value 2018-11-20T14:45:19.182Z cannot be converted to DateTime.