[Feature Request] Default/fallback value when an item is not available yet

[datvm]

Is your feature request related to a problem? Please describe.
Currently, when calling GetItemAsync<T>(key), a NullReferenceException is thrown if T is not nullable.

Describe the solution you'd like
There should be an overload for specifying the default value instead of throwing. For example:

GetItemAsync<T>(string key, T fallbackValue);
GetItemAsync<T>(string key, Func<T> fallbackValueFunc);
GetItemAsStringAsync(string key, string fallbackValue);
GetItemAsStringAsync(string key, Func<string> fallbackValue);

Describe alternatives you've considered
An alternative is to return null instead of throwing an exception like JsonSerializer.Deserialize<T> is doing though honestly that is bad because most of the time, you expect non-null values more.

My current workaround:

await GetItemAsync<T?>(key) ?? new(); // Or something to generate the fallback value

Additional context
None

[chrissainty]

Hi @datvm,

I don't see this being something we'd add. The reason T should be nullable is that a value might not be in local storage and we might return null. It doesn't seem much work for a developer to check for null and then decide what to do. Unless I'm missing something?

[datvm]

Hi,

After some consideration you are right that adding the methods is not helping much. Sorry I didn't make myself clear when I wrote the suggestion. Right now the signature of the function is:

ValueTask<T> GetItemAsync<T>(string key, CancellationToken cancellationToken = default);

which, with Nullable Reference Type, is a bit misleading because usually we don't expect a NullReferenceException anymore. I think it should instead be

ValueTask<T?> GetItemAsync<T>(string key, CancellationToken cancellationToken = default);

and the result may be null? This way, the compiler would raise the warning to developers that the result may be null, similar to JsonSerializer.Deserialize<T>?

Update: after further considering, this is a breaking change so I think it's unlikely?

[chrissainty]

Thanks for the clarification, @datvm. I agree, the method signature is wrong and misleading. You're also correct it would be a breaking change. I think this can be done, but I'd like to roll it up with any other breaking changes for the next major release.

[datvm]

I made #208 PR for this. So I realized the project did not have NRT to begin with so this is not actually a breaking change. Before it could have returned null anyway, we just didn't know it. I think it's also okay if you merge it for the next major release only.

While making the changes, I found an interesting change we may want to make as well: the KeyAsync(int index) method. Right now it returns string? (null if the index is out of range). Should we keep it as it is right now (Javascript convention, which means developers have to manually check for null), or we throw IndexOutOfRangeException like C# convention (extra code on C# side, this is breaking change).

[chrissainty]

I made #208 PR for this. So I realized the project did not have NRT to begin with so this is not actually a breaking change. Before it could have returned null anyway, we just didn't know it. I think it's also okay if you merge it for the next major release only.

I believe it's still a breaking change as developers could have warning set as errors. In which case marking the return type as nullable could trigger a NRT warning, or in their case, an error.

While making the changes, I found an interesting change we may want to make as well: the KeyAsync(int index) method. Right now it returns string? (null if the index is out of range). Should we keep it as it is right now (Javascript convention, which means developers have to manually check for null), or we throw IndexOutOfRangeException like C# convention (extra code on C# side, this is breaking change).

This is a great spot. Could you raise this as a separate issue? This would also count as a breaking change so at least we're batching them up! 😄