Remove null from return type of LoadAsync on GroupedDataLoader

[jnoordsij]

Product

Hot Chocolate

Is your feature request related to a problem?

While migrating to v14 I noticed the addition of nullability to the dataloader LoadAsync calls. For the BatchDataLoader the return types are now respectively TValue? and IReadOnlyList<TValue?> when loading one vs. multiple , which makes a lot of sense given the implementation (https://github.com/ChilliCream/graphql-platform/blob/main/src/GreenDonut/src/Core/BatchDataLoader.cs#L51-L67). Then there's the LoadRequiredAsync version which strips the nullability and throws instead, which is a great improvement I'd say.

However, for the GroupedDataLoader I'm a bit puzzled about this change. It's return types are now TValue[]? and IReadOnlyList<TValue[]?> respectively when loading one vs. multiple; however looking at the implementation (https://github.com/ChilliCream/graphql-platform/blob/main/src/GreenDonut/src/Core/GroupedDataLoader.cs#L46-L55) I just can't see how null will ever appear as a result here. It looks like if the resultLookup does not contain a value for the key, this will result in an empty array rather than null. Now one can of course once again restore the 'old' behavior by replacing all calls with LoadRequiredAsync, but this feels a bit superficial and unnecessary to me, given that in practical terms it should not change the behavior.

The solution you'd like

Alter the signature of LoadAsync on GroupedDataLoader in such a way that the respective return types are once again TValue[] and IReadOnlyList<TValue[]>.

For reference: I reported this on Slack earlier to no avail.

[michaelstaib]

They are based on the same base class. In general I would move away from the GroupedDataLoader and model them as BatchDataLoader instead. What we do in projects is simply define them as BatchDataLoader<int, Foo[]>.

[jnoordsij]

I'm not sure that solves the problem? When using BatchDataLoader<int, Foo[]> the result types will still be Foo[]? and IReadOnlyList<TValue[]?>. I think for grouped behavior, it makes little sense to allow for null, as an empty array makes much more sense as a fallback and ensures no further changes are required.

Of course using LoadRequiredAsync all across the codebase does solve this (like it did when using GroupedDataLoader), but that just feels really superfluous to me for this usecase.