-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix/optimize value coercion check for OneOf type #4181
Conversation
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hi @Cito, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
@Cito -- do you think there is some value in reporting multiple errors, i.e. if there are multiple keys, and some of them are null, multiple things are wrong? The code at present, heads toward that direction, checks the number of values, and checks the identity of the first, so you would get more detailed errors if there were two keys, first being null, but not more details errors if there were two keys with the second null. Or possibly I have the flow wrong! |
@Cito from feedback from @benjie on #4195, it seems like the correct style is one error per incorrect value, with the suggestion there to just include a more generic error message to catch all cases, so that if the incorrect number of keys are present or any are null, just emit an error message like:
So I think that's the suggestion I would make here. The idea of #4195 is to add the pre-coercion checks => your bug fix for not emitting more than one error could be landed separately, before that. What do you think? |
We can always combine the error messages in a separate PR. |
Where a JavaScript value is coerced to a GraphQL OneOf Input Type, a null value should only be reported when the "one of" condition is satisfied. The code block starting at line 158 here that accesses
keys[0]
should not be executed ifkeys
is empty or contains more than one item.The PR fixes this by adding an else statement. Alternatively, the "if/else" branches could be reversed, and the
keys.length !== 1
check should become akeys.length === 1
check.