rrousselGit / riverpod

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

Delay selectors & provider rebuilds until their first widget read #1371

Open rrousselGit opened 2 years ago

rrousselGit commented 1 year ago

One difficulty with regards to this feature is, it would make Riverpod fundamentally incompatible with context.watch – if we ever managed to convince the Flutter team to add the necessary bits to implement it.

The problem is that there is a bit of a clash between a few things:

Maybe a better alternative is:

Asking users to deal with an early abort of providers sounds a bit much though. Most people will likely forget it, and it's tedious.

iosephmagno commented 10 months ago

Hello @rrousselGit, just noticed this. We are being hit by janky screen animation in the scenario where we render a screen by fetching data from state via watch (data is a merely List<Something>. No issue with read. Testing with riverpod 2.4.9. Is there a way to delay the watch so that we can for example set delay to 600ms and watch will become effective only after screen animation is completed? Or any other potential solution to this?

rrousselGit commented 10 months ago

for example set delay to 600ms and watch

Delay the state update

Or any other potential solution to this?

Who knows. I don't know what's causing your jank. I'd suggest looking at that specifically. For instance maybe you should split your widget into smaller chunks to reduce the impact of a state change.

delfme commented 10 months ago

Thx! Will have a look at this. Indeed it is weird case. A junior coded it and might be just a minor mistake. The weird part is that janks dosent occur with read and we dont have any multiple rebuild. Hence, cannot see why watch is currently blocking and read is not.

rrousselGit commented 10 months ago

Keep in mind that rebuilds aren't limited to widgets. Maybe an expensive provider rebuilt. And providers don't rebuild if they aren't listed (which could happen if you use read instead of watch)

For example, maybe you have a provider that does a list.where(...).toList() on thousands of items, which could be expensive.

iosephmagno commented 10 months ago

@rrousselGit Thx. I checked the code and this might be a flutter bug.

Basically, we fetch messages from Sqlite, kMinMessageBatch is 25 and result is got in a few ms

 final messages = await _registry
     .get<LocalDatabase>()
     .getMessages(roomId, limit: kMinMessageBatch);
 _stateMessages.addAll(messages);

_stateMessages is our StateNotifierProvider and addAll(...) is okay:

// Add messages to state
void addAll(List<Message> messages) {
  state = [...state, ...messages];
}

I still get the issue if I sublist, ie. _stateMessages.addAll(messages.sublist(0,10)). So maybe this is a flutter engine issue?

A quick unorthodox hack I coded to workaround this is to use read and refresh UI at need via Provider notifyListeners().

Just to share thoughts, do you see doable/make-sense to delay watch ? Like: ref.watch(stateProvider, 250) and the widget will be in read mode for first 250ms and watch mode from 250ms on. This would be helpful to workaround janky screen animation when the state is updated exactly meanwhile a screen animation is going on.

Another feature that might be helpful in complex apps like ours is to make the provider only readable or only watchable and then allow to switch between the two modes programmatically like: StateNotifierProvider.AllowWatch(bool) and _stateMessages.AllowWatch(false) will disable watch (listeners will readonly), and vice-versa to enable watch back again.

rrousselGit commented 10 months ago

Delaying watch makes no sense, no.
Watch isn't the issue here. It's your list update.

Maybe you want to use a mutable list for instance. For large lists, a clone may not fit. You could do:

state.addAll(messages);
ref.notifyListeners();
iosephmagno commented 10 months ago

Discovered interesting things after testing below 3 solutions: Tested on iphone pro 13 ios 16.7.2, flutter master, 3.18.0-18.0.pre.39

1 RiverPod only: If I use ChangeNotifierProvider + watch, the screen animation will be janky if I do not delay notifyListeners(), and if I add a delay, it will be janky at times (like 1-2 times out of 10).

2 RiverPod + Provider: If I use StateNotifierProvider + read, and wrap the UI inside a Provider and delay Provider notifyListeners(), the screen animation will be smooth all times. Notice that now Provider is the only responsible of firing rebuilds.

3 Provider only: If I use Provider only and delay notifyListeners(), screen animation will be smooth all times.

I tested also with a simpler state, not a List, just simple Class. So issue is not related to the way we populate state but to some side effect of Riverpod watch due to maybe flutter issue.

Bottom line: In case of state being updated meanwhile a screen animation is going on, smoothness is guaranteed only by reading state (not watching) and then firing a rebuild via notifyListeners() after some delay (enough delay to let screen animation ends).

Im currently using solution 2, coz we access more providers from same place and Riverpod code is cleaner in that case. Also, we have migrated to Riverpod and makes no sense to move back to Provider. We will keep using Provider as a companion. I would have gone with solution 1, but due to janks ChangeNotifierProvider is currently not a substitute of Provider in our case.

rrousselGit commented 10 months ago

Sounds like you have a small example to benchmark this. Could you share it?

rrousselGit commented 10 months ago

Also please make a separate issue. As I told you before, delaying notification has nothing to do with performance.

If you found a performance issue, that should be treated separately. We can have optimizations without having a delay