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

Add more information in conversion error #1579

Closed
wants to merge 5 commits into from

Conversation

domi7777
Copy link

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.

@facebook-github-bot
Copy link

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!

@domi7777
Copy link
Author

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).

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

}.`,
}. Value ${inspect(result)} cannot be converted to ${inspect(
returnType,
)}`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)}`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Member

@IvanGoncharov IvanGoncharov Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>`);
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the advice, I'll do that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}`,
  );
}

Copy link
Member

@IvanGoncharov IvanGoncharov Nov 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IvanGoncharov - mind sharing some edge cases you can imagine?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@IvanGoncharov IvanGoncharov added the PR: bug fix 🐞 requires increase of "patch" version number label Feb 5, 2019
@IvanGoncharov IvanGoncharov added this to the v14.2.0 milestone Feb 5, 2019
Copy link
Contributor

@mjmahone mjmahone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a positive improvement from my end.

@IvanGoncharov IvanGoncharov removed this from the v14.x.x milestone Aug 13, 2020
Base automatically changed from master to main January 27, 2021 11:10
IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Aug 11, 2021
IvanGoncharov added a commit that referenced this pull request Aug 11, 2021
IvanGoncharov added a commit that referenced this pull request Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants