Let AsyncError clear the previous data

[trejdych]

trafficstars

Hi!

It would be nice to be able to clear providers' data explicitly for AsyncError state. To be fair I have only one use case when it would be useful but still. (I'm aware if my provider is disposed the data will be cleared.)

Let me share the sample

@riverpod
class ProfileController extends _$ProfileController {
  @override
  Future<Profile> build() async {
    if (!ref.watch(isAuthenticatedProvider)) {
      throw const UnathorizedException();
    }

    final me = await ref.watch(profileApiProvider).me();

    return me;
  }
}

in this case, it would be critical to expose profile data when a user is logged out. I can't* remove all listeners because the app works for non-authenticated users too. Of course, technically I can but it would require leaving a logic on the UI and it's easy to forget

Describe the solution you'd like The solution could be the ability to override the async notifier method. Pseudocode:

@override
  AsyncValue<Profile> asyncTransition(
    AsyncValue<Profile> newState, {
    required bool seamless,
  }) {
    if (newState.error is UnathorizedException) {
        return newState;
    }

    return super.asyncTransition(newState, seamless);
  }
// or 
@override
bool keepPreviousState() => state !is UnathorizedException;

Describe alternatives you've considered It's more than considered, it's my current production solution. But I don't like that solution. My profileController is a sync notifier with AsyncValue

@riverpod
FutureOr<Profile> _profile(Ref ref) async {
  final me = await ref.watch(profileApiProvider).me();

  return me;
}

@riverpod
class ProfileController extends _$ProfileController {
  @override
  AsyncValue<Profile> build() {
    if (!ref.watch(isAuthenticatedProvider)) {
      return AsyncError(const UnathorizedException(), StackTrace.current);
    }

    final profileState = ref.watch(_profileProvider);

    return profileState;
  }
...
}

[rrousselGit]

Your examples use Ref.watch, and as such already give the UI enough information to decide to not rely on the old data.

Cf AsyncValue.isRefreshing/isReloading

[trejdych]

Hmm, that's my point. I don't want to give the UI the responsibility to decide if data are old or forbidden. It's often fine to present them, like if you refresh data, but the request failed. And AsyncValue isn't always needed, sometimes it is convenient (and relatively safe) to use value/requiredValue

[rrousselGit]

I don't want to give the UI the responsibility to decide if data are old or forbidden

The responsibility is in your provider. It's your provider that set AsyncValue.isRefreshing/isReloading and such.
Your issue is that even though the provider did set those flags, your UI did not take them into consideration.

And AsyncValue isn't always needed, sometimes it is convenient (and relatively safe) to use value/requiredValue

asyncValue.asData()?.value should be the one-liner equivalent

[trejdych]

It's not the same. Easy to forget, new devs have to know about it. It trims all errors not only specific.

[rrousselGit]

You have all the information you need to have your UI behave as you expect to.
You can always make an extension on AsyncValue that's an alternative to AsyncValue.value but matches your behaviour:

extension<T> on AsyncValue<T> {
  T? get data => isReloading ? null : value;
}

I do get the concern of having a good default when using .value/.requireValue. But IMO the solution isn't at the notifier level.

I'd consider maybe splitting AsyncValue.value in two:

class AsyncValue<T> {
  // during isLoading: true, the previous value emitted
  T? get previousValue;

  // Null during isReloading: true.
  // T if AsyncData
  // T if isRefreshing: true
  T? get value;
}

[trejdych]

Still devs have to know about that extension and remember to use it for this specific provider. I think I prefer to stay with my sync provider workaround. Thanks though

[rrousselGit]

Don't close this. It is an oversight on my part that there is no easy way to handle isReloading gracefully. I'll make a breaking change on value/requireValue to exclude isReloading.

[trejdych]

Hmm but it wasn't only about isReloading It was mostly about AsyncError. I thought my suggestion would not be the breaking change

[AhmedLSayed9]

This is similar to #2097

I had a proposal for this before at https://github.com/rrousselGit/riverpod/issues/2102#issuecomment-1398790433:

state = AsyncValue.error('Dummy error', StackTrace.current, maintainPrevious: false);

But I'm not sure how this should work with build method

[rrousselGit]

Riverpod already has a concept for when the previous value should be ignored or not (reload vs refresh).

The issue isn't so much "we need a way to clear the previous state". The issue is that value/requireValue do not respect "refresh vs reload".

[AhmedLSayed9]

He doesn’t care much about showing prev data during loading since he's throwing the exception directly after the loading state anyway:

Future<Profile> build() async {
    if (!ref.watch(isAuthenticatedProvider)) {
      throw const UnathorizedException();
    }
  }

I think the issue is more about clearing previous data when moving from Data to Error, so devs don’t accidentally use prev data while the provider is in error state.

While asyncValue.asData()?.value/valueOrNull can be used in the UI, it’s easy to miss when used in different places by the developers. He’s looking for a way to reset the data from the provider’s state itself.

[rrousselGit]

As I said, "previous state clear" is already a thing. Since he's using ref.watch, the previous state is already marked as "probably shouldn't use it" (isReloading).
His true problem is that he used AsyncValue.value, but .value didn't respect the fact that the previous value shouldn't be used.

His code was supposed to behave how he expected from the get go. He shouldn't need to change his provider at all to "clear the previous state", since that's directly integrated with ref.watch.

It's just an AsyncValue issue.

[AhmedLSayed9]

I understand your point, but what I'm saying is that excluding value/requireValue during reloading is unrelated to what @trejdych needs. He needs .value to be null when in error state while it has previous data (and also to prevent access to previous data "when in error state" through .requireValue, .when, or any other API).

[AhmedLSayed9]

The default behavior of .when does not return the previous data when in an error state or while reloading.
but, the other three APIs (value, requireValue, and valueOrNull) return the previous data in those cases.

imo, changing all these APIs' behavior or making new ones is a big change and doesn’t really prevent devs from mistakenly accessing prev data.
Anyway, users can currently do .unwrapPrevious()?.value/.valueOrNull if they don’t want to return prev value during loading/error states in some place.

But to prevent it entirely, I’m more in favor of offering a way to clear value when entering loading/error state via the provider itself. This could be optional, maybe two flags like preserveDataOnLoading/preserveDataOnError on the Riverpod annotation or something similar.

[trejdych]

This is similar to #2097

I had a proposal for this before at https://github.com/rrousselGit/riverpod/issues/2102#issuecomment-1398790433:

state = AsyncValue.error('Dummy error', StackTrace.current, maintainPrevious: false);

But I'm not sure how this should work with build method

Yep basically I would like to have sth like that but for build method ;)

[rrousselGit]

I understand your point, but what I'm saying is that excluding value/requireValue during reloading is unrelated to what @trejdych needs. He needs .value to be null when in error state while it has previous data (and also to prevent access to previous data "when in error state" through .requireValue, .when, or any other API).

It's not unrelated

This is exactly what I'm saying the breaking change on .value based off isReloading should be.

[rrousselGit]

Bear in mind: In a previous version, value would be null in those cases.

Ref.watch used to set value to null. But it was changed because some folks wanted a way to access the old state anyway

I'm saying that we need a middle ground: Have value be a good default, and still offer a mean to check the old state.

[AhmedLSayed9]

I'm saying that we need a middle ground: Have value be a good default, and still offer a mean to check the old state.

I don't think having another api to check old state separately is a good idea. Users will have to check i.e valueOrNull ?? oldValueOrNull and will be more complicated with value/requireValue.

Maybe keep the current value/requireValue/valueOrNull and provide other apis currentValue/currentValueOrNull/...? but this means the default ones will still return value on error and reloading.

Instead, we could convert those getters into methods and offer optional flags similar to .when, i.e (we can find better naming):

value({bool skipLoadingOnReload = false, bool skipLoadingOnRefresh = true, bool skipError = false})

This makes the behavior clearer, more flexible, and easier to reason about through flags.

Additionally, unlike .when, I believe skipLoadingOnReload should default to true here. It’s very common to use something like final x = ref.watch(provider).value / requireValue where the provider might reload as its dependencies change. In such situations, this would throw and break things whereas .when would simply transition to a loading state. I reviewed one of my projects that diversely use requireValue/value/valueOrNull across both UI and providers, and I couldn’t find a single case where I’d want skipLoadingOnReload to be false.

On the other hand, defaulting skipError to false feels reasonable and should almost cover @trejdych’s use-case. I say almost because he was asking for a stricter approach.

His concern is preventing developers from being able to call asyncValue.value(skipError: true) or access previous data via any API. He wants to clear it from the source itself. Maybe this could be made configurable via flags like preserveDataOnLoading / preserveDataOnError on the Riverpod annotation itself. I think this can be useful in other use cases too where leaking prev data is critical and should be forbidden.

[trejdych]

Adding the flag to the annotation will be code generation specific and skip all errors. I don't mind preserving data on error. It is a great concept. My concern is keeping data when it should not like AsyncError(error: "User logged out", value: Profile(name: "Tomasz Rejdych"). At least for now, it's my only use case but that's why I think it could be configurable.

[rrousselGit]

Maybe keep the current value/requireValue/valueOrNull and provide other apis currentValue/currentValueOrNull/...? but this means the default ones will still return value on error and reloading.

That's problematic, because people use value/requireValue.

I want the default behavior to be the most logical one ; which is "value should be null during updates caused by ref.watch".

For starter, I want the following to be a good way to do pattern matching:

switch (asyncValue) {
  AyncValue(:final value?) => 
  AsyncValue(:final error?) => 
  _ => 
}

And for that to be a good default, we need value to be null during ref.watch updates.

[AhmedLSayed9]

I want the default behavior to be the most logical one.

I agree on that.

Maybe we can do that for value, but I don't see the point for requireValue to throw if the provider reload.

Also, are we talking about value to be null on error too? As this is the main concern of the issue.

[rrousselGit]

It's unrelated to AsyncLoading vs AsyncError. It's related to what caused the refresh (ref.watch). So yes, value would be null on error too.

requireValue is just value! with a better error message. So if value is null, requireValue should throw.

[AhmedLSayed9]

Alright. That would make requireValue a bit harder to work with.

In most of my scenarios where I use requireValue, the provider might reload. like in this case:

@riverpod
Future<ProductCategories> productCategories(Ref ref) {
  ref.watch(selectedBranchProvider);
  return ref.watch(menuRepoProvider).fetchProductCategories();
}

@riverpod
FutureOr<ProductCategory> productCategoryDetails(Ref ref, String categoryId) {
  final categories = ref.watch(productCategoriesProvider);
  return categories.requireValue.items.firstWhere((c) => c.id == categoryId);
}

productCategoriesProvider can reload when selectedBranchProvider changes. But when that happens, productCategoryDetailsProvider will enter an error state.

I've other use cases in the UI too (where UI won't handle errors).

It might be nicer to let users tweak the behavior with flags on value/requireValue, while keeping good defaults in place.

Also, I think we should make a new issue for this with more details and ask folks to share their opinion?

[rrousselGit]

This kind of specifics is to technical to gather proper opinion.
But I'll also add a valueOrPrevious/requireValueOrPrevious alongside the change.

Your code is non-standard I'd say. For starter, you're expected to use await ref.watch(productCategoriesProvider). Those who don't are likely niche.

[AhmedLSayed9]

productCategoryDetailsProvider always comes after productCategoriesProvider is loaded. There are other, more conventional examples, but I think it’d be totally fine if we're going to have an option to require value or prev.

What about valueOrNull? valueOrNullOrPrevious sounds weird 👀

What’s your take on using methods with options for this instead? I know it’d be a breaking change, but it might be better in the long run and help reduce confusion around the different APIs.

[rrousselGit]

Methods are incompatible with switch Can't do:

case AsyncValue(:final value())

What about valueOrNull? valueOrNullOrPrevious sounds weird 👀

value is valueOrNull. valueOrNull has been renamed.

So just valueOrPrevious

[AhmedLSayed9]

So just valueOrPrevious

If there's no previous value, will that return null on error or rethrows the error? I think that's the diff between value and valueOrNull

[rrousselGit]

null

As I said, valueOrNull was renamed to value. There's no more "reading value may throw"

[AhmedLSayed9]

Oh, it's there on dev.

Great! I've never needed to use .value tbh.

To summarize, we might introduce this in the future:

• T? value → Returns the current value or the previous value when refreshing. Returns null during initial load, reload, or when in an error state.
• T requireValue → Returns the current value or the previous value when refreshing. Rethrows the error if in an error state, and throws a StateError during initial load or reload.
• T? valueOrPrevious → Returns the current value or previous value if in loading/error state. If neither exists, returns null.
• T requireValueOrPrevious → Returns the current value or previous value if in loading/error state. If neither exists, rethrows the error if in an error state, and throws a StateError if in loading state.

[rrousselGit]

Yes. With the valueOrNull -> value renaming, does it make more sense as to why I think it's logical to change value's behavior here? :)