[Discussion] skipLoadingOnReload should be set to TRUE by default

[T-moz]

skipLoadingOnReload should be set to TRUE by default

It's not really a bug, but rather a design improvement.

In the preview version 2.0.0-dev.0 you introduced the property isRefreshing to AsyncData. Witch is freaking awesome 🎉 !

This version was in active development, and you changed the internal behaviour. Now AsyncLoading can hold data.
If user define a unique FutureProvider, this was not a problem. When the FutureProvider is refreshed or invalidated, it will emit an AsyncData and the property isRefreshing will be true.

But, if the FutureProvider depends on an other Provider and this one is refreshed, then the FutureProvider will first emit a AsyncLoading and then an AsyncData.

To let users know when the data is reloading you added the argument skipLoadingOnReload to the when method of AsyncValue.

I think that this design has a few caveats:

• .when has more functionalities that .map.
  • Not every Riverpod users are aware of those differences.
  • Some AsyncValue getters relie on .map and not .when.
• Maybe it's just me, but I don't see any use case where skipLoadingOnReload should be set to false. So I have to set skipLoadingOnReload = true every time I use an AsyncValue.

I have also found two inconsistencies caused by this design.

Refreshing and Reloading do not share the same behaviour

Refreshing a Future provider doesn't not emit a new AsyncLoading. But refreshing a provider on which the future provider depends emits a new AsyncLoading.

final randomDoubleProvider = Provider<double>(
  (ref) => Random().nextDouble(),
);

final labelProvider = FutureProvider<String>(
  (ref) async {
    final randomDouble = ref.watch(randomDoubleProvider);
    await Future.delayed(Duration(milliseconds: 100));
    return 'double is $randomDouble';
  },
  dependencies: [randomDoubleProvider],
);

void main() {
  /// The two providers are dependent like this:
  ///
  /// ┌────────────────────────┐
  /// │        Provider        │
  /// │                        │
  /// │                        │
  /// │      randomDouble      │
  /// │                        │
  /// └───────────┬────────────┘
  ///             │
  ///             │
  ///             │
  ///             │
  ///             │
  /// ┌───────────▼────────────┐
  /// │     FutureProvider     │
  /// │                        │
  /// │                        │
  /// │                        │
  /// │         label          │
  /// │                        │
  /// └────────────────────────┘
  ///
  /// * Refreshing is when the second provider is `refreshed`.
  /// * Reloading is when the first provider is `refreshed`.
  test('Refreshind and realoading behave the same', () async {
    // Instanciate two counters
    int asyncLoadingCountWhenRefresh = 0;
    int asyncLoadingCountWhenReload = 0;

    // Instanciate two containers
    final containerForRefresh = ProviderContainer();
    final containerForReload = ProviderContainer();

    containerForRefresh.listen(labelProvider, (previous, next) {
      if (next is AsyncLoading) asyncLoadingCountWhenRefresh += 1;
    }, fireImmediately: true);

    containerForReload.listen(labelProvider, (previous, next) {
      if (next is AsyncLoading) asyncLoadingCountWhenReload += 1;
    }, fireImmediately: true);

    // Read the future provider
    await containerForRefresh.read(labelProvider.future);
    await containerForReload.read(labelProvider.future);

    // Refresh
    containerForRefresh.refresh(labelProvider);
    // Reload
    containerForReload.refresh(randomDoubleProvider);

    // Read the future provider again
    await containerForRefresh.read(labelProvider.future);
    await containerForReload.read(labelProvider.future);

    // Expect behaviour (number of `AsyncLoading` emitted) to be the same
   // Will fail in version 2.3.4
    expect(asyncLoadingCountWhenRefresh, asyncLoadingCountWhenReload);
  });

}

Stream and Future providers do not share the same behaviour

• When the dependency of a StreamProvider changes, the StreamProvider will emit a new AsyncData.
• When the dependency of a FutureProvider changes, the FutureProvider will emit a new AsyncLoading.

This can lead to tricky bugs when refactoring code, following the depreciation of the .stream property.

final randomDoubleStreamProvider = StreamProvider<double>(
  (ref) async* {
    for (var i = 0; i <= 2; i++) {
      await Future.delayed(Duration(milliseconds: 100));
      yield Random().nextDouble();
    }
  },
);

final labelStreamProvider = StreamProvider<String>(
  (ref) {
    final randomDoubleStream = ref.watch(randomDoubleStreamProvider.stream);
    return randomDoubleStream
        .asyncMap((randomDouble) => 'double is $randomDouble');
  },
  dependencies: [randomDoubleStreamProvider],
);

final labelFutureProvider = FutureProvider<String>(
  (ref) async {
    final randomDouble = await ref.watch(randomDoubleStreamProvider.future);
    return 'double is $randomDouble';
  },
  dependencies: [randomDoubleStreamProvider],
);

void main() {
  /// The three providers are dependent like this:
  ///
  ///                ┌─────────────────────┐
  ///                │    StreamProvider   │
  ///                │                     │
  ///           ┌────┤                     ├───┐
  ///           │    │     randomDouble    │   │
  ///           │    │                     │   │
  ///           │    └─────────────────────┘   │
  ///           │                              │
  ///           │                              │
  ///           │                              │
  ///           │                              │
  ///           │                              │
  /// ┌─────────▼──────────┐        ┌──────────▼─────────┐
  /// │   FutureProvider   │        │   StreamProvider   │
  /// │                    │        │                    │
  /// │                    │        │                    │
  /// │                    │        │                    │
  /// │       label        │        │        label       │
  /// │                    │        │                    │
  /// └────────────────────┘        └────────────────────┘
  test('Stream and Future provider have the same behaviour', () async {
    // Instanciate two counters
    int asyncLoadingCountFromStreamProvider = 0;
    int asyncLoadingCountFromFutureProvider = 0;

    // Instanciate the container
    final container = ProviderContainer();

    container.listen(labelStreamProvider, (previous, next) {
      if (next is AsyncLoading) asyncLoadingCountFromStreamProvider += 1;
    }, fireImmediately: true);
    container.listen(labelFutureProvider, (previous, next) {
      if (next is AsyncLoading) asyncLoadingCountFromFutureProvider += 1;
    }, fireImmediately: true);

    // Read the future and stream providers
    await container.read(labelStreamProvider.future);
    await container.read(labelFutureProvider.future);

    // Await for a new random number
    await Future.delayed(Duration(milliseconds: 150));

    // Read the future and stream providers again
    await container.read(labelStreamProvider.future);
    await container.read(labelFutureProvider.future);

    // Expect behaviour of stream and future providers (number of `AsyncLoading`
    // emitted) to be the same.
    // Will fail using version 2.3.4
    expect(
      asyncLoadingCountFromStreamProvider,
      asyncLoadingCountFromFutureProvider,
    );
  });
}

Exemples:

In this repo, there are exemples of those two problems.
Both problems are illustrated using Riverpod version 2.0.0-dev.0 and 2.3.4.

---

Expected behaviour

I suggest that refreshing a provider should not cause the future providers that depend on it, to emit a AsyncLoading.

---

Riverpod rocks 🤘
Thanks for all the work you put in this project.

[rrousselGit]

There are a few dangers with having it true by default. This can cause your UI to render a bad state.

Say you keep the user ID in a provider, and you have a FutureProvider watching that userIdProivder to do some network requests. Such that when the user ID changes, the FutureProvider automatically updates through ref.watch.

In that scenario, having the reload flag to true by default could cause possible security concerns.
Say in the app, you switched from user A to user B. In that event, with skipLoadingOnReload:true, your UI would show the result of the network request performed with user A, even though you're logged with user B.

To avoid possible problems like this, the flag is opt-in on purpose.

---

Refreshing a Future provider doesn't not emit a new AsyncLoading. But refreshing a provider on which the future provider depends emits a new AsyncLoading.

That's expected. In the latter case, a dependency of the provider changed. As such, it's a reload, not a refresh. That's the whole point of the different between "reload" vs "refresh"

Stream and Future providers do not share the same behaviour

• When the dependency of a StreamProvider changes, the StreamProvider will emit a new AsyncData.
• When the dependency of a FutureProvider changes, the FutureProvider will emit a new AsyncLoading.

That isn't the case. Both would emit AsyncLoading

Your sample isn't showcasing a dependency change. Your StreamProvider didn't rebuild.
To begin with, streamProvider.stream is deprecated as the way you used it creates a race condition in which your code is bugged. You shouldn't use .stream :)

---

In the end, if you want to have the flag on by default, you're free to make an extension on AsyncValue to make a custom "when" with it pre-enabled

extension<T> on AsyncValue<T> {
  R myWhen<R>({
    required R Function(T data) data,
    required R Function(Object error, StackTrace stacktrace) error,
    required R Function() loading,
  }) {
    return when(skipLoadingOnReload: true, data: data, error: error, loading: loading);
  }
}

[T-moz]

Thanks for your response.

I now better understand why the flag is set to false by default 🧑‍🎓