Make some standard fields non nullable

[ahey]

Generating a graphql layer for my front-end in typescript, I noticed some fields are unnecessarily nullable. To achieve a more pleasant typescript experience, I propose the following fields be made non null:

• errors as returned by a mutation. In case of no errors, an empty list will be returned. Furthermore, each item in the error list can also be made not null, since it either exists in the list or not, and we don't want nulls in the list.
• error.fields

In addition to these fields, I would also like to see the the _result fields be made non nullable, as they will always contain the result and errors fields, except in the case of root_level_errors? being enabled, in which case i suppose the _result field should be nullable.

See here for an example of the changes (not ready for merge, just for discussion)

This would be a breaking change to the existing api. We could keep the existing behaviour as default, and add a config setting to enable the new behaviour. The existing behaviour should be considered deprecated and we could warn to use the flag, and default to use it in v1.0.0.

What do you think?

[zachdaniel]

IIRC result will be null if errors is present, and errors will be null if the result is present, right? However, items in the error list being non null can definitely be done.

[rbino]

I think (also from the diff) that @ahey was referring to the fact that the Result type as a whole is non-nullable (as you said you always either have a result or some errors), so

createFoo(input: CreateFooInput): CreateFooResult

should instead be

createFoo(input: CreateFooInput): CreateFooResult!

This (as mentioned above) is particularly useful when integrating with TypeScript where nullability is important for type generation.

Another doubt I have is: should the input also be non-nullable?

[zachdaniel]

Ah, yes you are right, we can definitely make that return type non nullable. Going from nullable -> not nullable on a returned type isn't breaking, so I'd definitely accept a PR on that front. As for non-nullable input, that one is more tricky. Not all actions require some kind of input, and if all keys are not required, the input could also be not required. It would be a breaking change to require it, even if we only did it in cases where there was at least one required input inside of it, because nullable -> not nullable on an input type is theoretically breaking. Although in those scenarios the action would fail 100% of the time... 🤷 so maybe fine to just make both changes. overall result type not nullable, and input not nullable only if at least one member is not nullable.

[barnabasJ]

I just updated and stumbled upon this because my test now failed with data being now nil instead of it being %{"<operationName>" => nil}; I was a bit confused at first because a couple of lines above I was still getting something like this. But it was a read_one query, maybe to be consistent, the return type should be non_nullable for those too? Same for get probably.

At least I think if there is no error there should always be a record returned, otherwise there will be a NotFound Error?

[zachdaniel]

@barnabasJ does this solve the issue? #110

[barnabasJ]

@barnabasJ does this solve the issue? #110

#110 introduced the change and #114 reverted it back to the old behaviour for me

[ahey]

Hey @rbino thanks for rolling with this one! Is it safe to say that this is complete? If so I will close the issue.

[ahey]

I found another area for improvement, which from my testing appears to be backwards compatible. I have made a PR with the changes in make mutation result errors list non-nullable. Can anyone forsee any issues with this?

My understanding is that moving from a nullable list of errors to a non-nullable list of errors is safe, since we always return an empty array in case of no errors.