Add exception handling to 'ref.refresh()'

[snapsl]

Is your feature request related to a problem? Please describe. #3651

Describe the solution you'd like Here is how riverpod currently recomments implementing a pull to refresh.

RefreshIndicator(
   onRefresh: () async => ref.refresh(provider.future),
   child: ...
)

However, this leads to an exception at the UI level if reading the updated value fails. The “ref.refresh()” function should take care of this.

Instead of returning a value, ref.refresh() should be handled similarly to a mutation that is watched by ref.watch(provider) and can be listened to by ref.listen(provider). Therefore, it does not need to return an actual value, but indicate when the operation has finished (see RefreshIndicator.onRefresh).

Options:

• ref.refresh() should return a Future<void> / void / FutureOr<void>
• ref.refresh() returns a value? only containing a value if the refresh was successful
• add bool arg throwOnError to ref.refresh()

Describe alternatives you've considered Users should implement their own exception handling. But this needs to be explicitly mentioned in the API reference. Also, the examples in the documentation should be updated accordingly.

[rrousselGit]

There's nothing wrong with the future throwing IMO. You're not even awaiting it. That's similar to ref.watch(provider.future)

Why does that matter?

[snapsl]

You're not even awaiting it.

Thanks...

Uncaught exceptions in callbacks are forwarded to the PlatformDispatcher (link). If these are not handled properly, such “errors” are logged for no reason.

[rrousselGit]

There is an error in your code afterall. Why would we want to silence it? If you want to silence it for the sake of error logging, you can do refresh(...)..ignore()

[snapsl]

Yes, the error is that the rebuild of the provider failed, e.g. due to a failed network request. This is logged by provider observer or the network service. However, since ref.refresh triggers an exception, there is an additional error at the UI level, but it is not a UI problem. This might be nitpicky and .ignore() would solve this. However, I think ref.refresh() should handle this out of the box.

[rrousselGit]

This isn't up to ref.refresh. If anything, it's up to RefreshIndicator.
You likely want a RefreshIndicator that does not report errors as "uncaught". Because RefreshIndicator explicitly decided to report the error

You could make a wrapper around it:

class MyIndicator extends StatelessWidget {
  Future<void> Function() onRefresh;

  build(ctx) {
    return RefreshIndicator(onRefresh: () => onRefresh()..ignore(), child: child);
  }
}

[snapsl]

Hi @rrousselGit, thank you for taking your time and reviewing this feature request. I understand that this is not a critical issue. Maybe some people also rely on refresh to return a value. For me, its usage was always for (async) callbacks.

Feel free to close this.

Some suggestions:

1. We could update the API reference to mention that refresh can throw exceptions. This was not obvious to me and certainly not to others either.
2. When implementing mutations, consider adding exception handling to them as they are also frequently used in callbacks.

[rrousselGit]

We could update the API reference to mention that refresh can throw exceptions. This was not obvious to me and certainly not to others either.

It is part of the documentation of provider.future that it will rethrow any error.
This is unrelated to ref.refresh.

Changing provider.future to return a Result-like object would be a massive breaking change, as it's very common for folks to do:

await ref.watch(provider.future);

In this scenario, it is very much expected that any error thrown by provider is rethrown.

For now, I'd rather not do such a change.