rrousselGit / riverpod

A reactive caching and data-binding framework. Riverpod makes working with asynchronous code a breeze.
https://riverpod.dev
MIT License
6.21k stars 948 forks source link

FutureProvider with ChangeProvider/StateNotifierProvider #962

Closed marinkobabic closed 2 years ago

marinkobabic commented 2 years ago

Problem

In the most cases we are

  1. Reading data by using a repository async methods. We use therefore FutureProvider. This works perfect: user can see thanks to the AsyncValue loading, error and the data
  2. Once the data is there we would like to use ChangeProvider, but this works only in a clean way if additional ProviderScope is used or the data is immutable and the ChangeFamilyProvider is used. Being immutable does not fit quite well to the idea of the ChangeProvider.

The idea is to not rebuild the FutureProvider when the ViewModel watched by the ChangeProvider changes. So we have here two different consumer widgets.

This pattern is valid also for the StateNotifierProvider, but the state should not be changed in the build method and all this stuff is happening inside of the build method.

The separation of layers looks in our architecture as following:

So by having this architecture in place you may think

I complex cases this patters leads to very ugly code where we need to build a complex object in order to pass all the models down to the ChangeNotifierProviderFamily.

Possible solution

FutureProvider returns the complete ViewModel and this is possible by using the existing framework. Now we need the ability to tell the Consumer to listen to the changes of the ChangeNotifier. This could be achieved if you would have a new Provider like the ChangeNotifierProviderFamily but where the parameter is not becoming part of the state so that object can change. Maybe every provider should support "future" like it supports "autodispose" and "family". Then ChangeNotifierProvider could be use to load the data initially in an async way and from that point on Consumer will watch for the changes.

Best practice

In generally it seems like there is a lot of confusion how to deal with such a situation. Community is coming up with solutions like StateNotifierProvider<AsyncValue> in order to load the data in an async way and to have the full control how to make the Consumer rebuild the widget. FamilyProvider should never be used for complex types even if possible when keeping them immutable but this makes the whole solution error prone and not maintainable. So what is your view on this?

rrousselGit commented 2 years ago

I'm having trouble understanding what this is about.

Are you returning a ChangeNotifier instead a FutureProvider? If so, why can't you use a ChangeNotiferProvider?

marinkobabic commented 2 years ago

FutureProvider returns the ViewModel. Doing changes on the returned ViewModel will not cause the consumer to rebuild the widget because it has no idea that it should listen to the ViewModel changes. On other side I can't return ChangeNotifierProvider by using the FutureProvider AsyncValue capabilities.

rrousselGit commented 2 years ago

Why? What prevents you from using AsyncValue inside a ChangeNotifierProvider/StateNotifierProvider?

marinkobabic commented 2 years ago

The question is would the code not look much better when all the providers would support "future"?

you mean like in the following example:

Future<int> fetch() aynsc => 42;

class Whatever extends StateNotifier<AsyncValue<int>> {
  Whatever(): super(const AsyncValue.loading()) {
    _fetch();
  }

  Future<void> _fetch() async {
    state = const AsyncValue.loading();
    state = await AsyncValue.guard(() => fetch());
  }
}

Then I have to take AsyncValue like in the FutureProvider in order to display different states? This would work, but then it makes the things more complicated to combine multiple providers to load the final data and to watch to those changes.

rrousselGit commented 2 years ago

Making all providers support futures is not something easily doable

One problem is that it would delay the creation of the StateNotifier/ChangeNotifier

So say you wanted to increment a counter, then you'd have to await for the StateNotifier to be created. That's undesired.

marinkobabic commented 2 years ago

I see your point. I would in generally suggest to add this pattern to the documentation so that everybody knows how to deal with such a situation. Will do some prototyping to find a way to keep the code as clean as possible when I need to handle multiple async requests from different sources. Many thanks. I would close the issue except you wan't to add documentation tag on it?

rrousselGit commented 2 years ago

Do note that I have some long-term plans that would fix this issue This would involve a new syntax for defining a notifier. It'd no-longer be a StateNotifier but rather something slightly different which would allow supporting async/await during initialization.

I'll close this issue in the meantime. There are other issues about async initialization of StateNotifiers, so there's no point in having multiple about them