riverpod icon indicating copy to clipboard operation
riverpod copied to clipboard

Delay selectors & provider rebuilds until their first widget read

Open rrousselGit opened this issue 2 years ago • 10 comments

rrousselGit avatar Apr 23 '22 06:04 rrousselGit

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:

  • we want ref.listen. In particular, we want ref.listen to be triggered in a context where it's safe to show snackbars/modals
  • we want selectors to be able to correctly filter rebuilds of a widget if the resulting value didn't change. So we can't ask all dependent consumers to rebuild when a provider wants to rebuild, only to rebuild the provider once the first consumer did in fact rebuild.
  • At the same time, it's possible that when a provider it about to rebuild, its consumers will in fact be disposed within the same frame. In which case we don't care about the rebuild anymore.

Maybe a better alternative is:

  • for selectors, catch errors on change https://github.com/rrousselGit/riverpod/issues/2653
  • for providers, suggest users to handle the scenario where a provider rebuilds but won't be used.

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.

rrousselGit avatar Jun 15 '23 10:06 rrousselGit

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?

iosephmagno avatar Jan 04 '24 15:01 iosephmagno

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.

rrousselGit avatar Jan 04 '24 15:01 rrousselGit

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.

delfme avatar Jan 05 '24 15:01 delfme

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.

rrousselGit avatar Jan 05 '24 16:01 rrousselGit

@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.

iosephmagno avatar Jan 06 '24 17:01 iosephmagno

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();

rrousselGit avatar Jan 06 '24 20:01 rrousselGit

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.

iosephmagno avatar Jan 07 '24 13:01 iosephmagno

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

rrousselGit avatar Jan 08 '24 10:01 rrousselGit

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

rrousselGit avatar Jan 08 '24 10:01 rrousselGit