rrousselGit / riverpod

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

Auto dispose after first listener #1329

Closed talent-apps closed 2 years ago

talent-apps commented 2 years ago

Riverpod version 2.0.0-dev.4 adds a disposeDelay feature to all autoDispose providers and to ProviderContainer/ProviderScope. This configures the amount of time before a provider is disposed when it is not listened.

To avoid timing issues, it would be nice to add a "auto dispose after the provider is listened to once" mode, which make the provider auto disposable only after it is listened to once.

Example:

rrousselGit commented 2 years ago
  • Only screen B watches the provider.

I would suggest have A listen to the provider until navigating to B

talent-apps commented 2 years ago

The nature of the use case is that A is not interested in this provider. A classic example is a master/details scenario:

This scenario is very common and the library doesn't seem to provide an easy way to handle it.

rrousselGit commented 2 years ago

Maybe it's worth expanding on what "initializing with some initial value" means and also explain what's the problem you're currently facing

talent-apps commented 2 years ago

The provider will be initalized with the item that was selected in the list. Then the app will navigate to screen B where you can edit the properties of the selected item.

rrousselGit commented 2 years ago

The provider will be initalized with the item that was selected in the list.

Right, but what exactly does this involve?

talent-apps commented 2 years ago

Code example:

void onItemSelected(BuildContext context, WidgetRef ref, Item selectedItem) {
  ref.read(editedItemProvider.notifier).setItem(selectedItem);
  Navigator.of(context).pushNamed(screenB);
}
class Item {
  final String name;
  const Item(this.name);

  Item copyWith(String? name) => Item(name ?? this.name);
}

final editedItemProvider = StateNotifierProvider.autoDispose<EditedItemNotifier, Item?>((ref) {
  return EditedItemNotifier(ref: ref);
});

class EditedItemNotifier extends StateNotifier<Item?> {
  final Ref ref;

  EditedItemNotifier({required this.ref}) : super(null);

  void setItem(Item item) {
    state = item;
  }

  void updateName(String newName) {
    state = state?.copyWith(newName);
  }
}
talent-apps commented 2 years ago

With the current implementation, editedItemProvider will be disposed before screen B is launched, even though screen B watches this provider.

rrousselGit commented 2 years ago

And why don't you want to set disposeDelay in this case?

talent-apps commented 2 years ago

It would work, but relying on a specific timeout is more error prone, while relying on adding/removing a listener is exact. With a timeout you also hold the provider state longer than you actually need it, which is less efficient. This scenario is very common in many apps and it would be nice to have this if it's a small feature to implement.

robpot95 commented 2 years ago

I agree in this case, even tho disposeDelay help the issuw. Still uncertain how long it should hold the state. There is cases when I switch screen and then back to the old screen. I would like to dispose it immediately and not delay the dispose.

talent-apps commented 2 years ago

Cool. An automatic disposal after at least one listener would help addressing issues like that.

danielkerwin commented 2 years ago

I've been dealing with this same issue - my use-case is initializing a AutoDisposing StateNotifierProvider that is used to cache form values until submission - the initialization occurs when "master" form widget is loaded, but this widget does not care about the state of the provider; the detailed form widgets subscribe to different parts of the form state via the provider.

I cannot use auto-dispose unless the master watches the provider, causing rebuilds in the entire tree - having the auto-dispose occur only after at least one listen would solve my issue.

talent-apps commented 2 years ago

Indeed this scenario is very common.

rrousselGit commented 2 years ago

Well ultimately you can do that yourself already

Provider.autoDispose((ref) {
  ref.keepAlive();
  ref.onCancel(ref.invalidateSelf);
}

This effectively produces the same result as autoDispose, without the state getting destroyed during navigation

robpot95 commented 2 years ago

Well ultimately you can do that yourself already

Provider((ref) {
  ref.onCancel(ref.invalidateSelf);
}

This effectively produces the same result as autoDispose, without the state getting destroyed during navigation

When will it get destroyed then?

rrousselGit commented 2 years ago

While the proposal is interesting, I'm not sure it's ultimately a good idea.

I could see a few flaws:

rrousselGit commented 2 years ago

When will it get destroyed then?

When all listeners are removed.

rrousselGit commented 2 years ago

If we want to do something other than disposeDelay (which I personally think is good enough for the problem described), I'd personally prefer having a way to make A temporarily listen to the provider until B is poped

Like maybe:

Button(
  onTap: () async {
    final sub = ref.listen(provider, (prev, value) {});
    await Navigator.push(); 
    sub.close();
  },
)
talent-apps commented 2 years ago

@rrousselGit this feels a bit awkward since you listen to something that you are not interested it. One more thing, the fact that this feature doesn't solve ALL scenarios is irrrelavnt since it obsivously helps solve some common scenarios. How exactly can we achieve this behaviour ourselves with the current implementation?

robpot95 commented 2 years ago

While the proposal is interesting, I'm not sure it's ultimately a good idea.

I could see a few flaws:

  • what if the provider is never listened?
  • what if you have:

    final a = Provider.autoDispose(..., waitUntilListenedBeforeDispose);
    
    final b = Provider((ref) { 
      ref.watch(a);
      return Something();
    });
    
    // inside some widget:
    ref.read(b);
    Navigator.push();

    That'd still cause a to be disposed, since it's temporarily listened by b

I agree with you, that this will cause a issue. But tbh when i set the provider state and navigate screen. First problem i'm bit unsure how long the disposeDelay should last. Maybe navigate screen takes a microsecond or couple seconds. Then i have the problem with that i would like to use the state in screen B that is setted from screen A. But i would like to dispose the state immediately after i leave screen B and not delay it.

Suggestion would be to autoDispose the state, if it's never watched after some time. But if it's being watched once, then it should autoDipose immediately.

talent-apps commented 2 years ago

@rrousselGit As for your example:

rrousselGit commented 2 years ago

Maybe navigate screen takes a microsecond or couple seconds

Navigating to a screen is not duration based. It's event-loop based. It'll take one cycle of the event loop

As such, any non-zero duration will do.

Also, "disposing immediately" doesn't matter. What's wrong with delaying the dispose by a few seconds?

rrousselGit commented 2 years ago

One more thing, the fact that this feature doesn't solve ALL scenarios is irrrelavnt since it obsivously helps solve some common scenarios.

It is important, since there are workarounds. Adding a new API comes with a significant cost

How exactly can we achieve this behaviour ourselves with the current implementation?

You can do the ref.listen variant I showed by doing:

final container = ProviderScope.containerOf(context);
final sub = container.listen(...);
await push();
sub.close();

This works today.

And I plan on streamlining this a bit by adding a WidgetRef.listen variant which returns the subscription instead of being used inside build like the current one.

danielkerwin commented 2 years ago

I agree with others that a temporary listener, or time based condition, feel awkward - what if the detailed components that subscribe to the provider themselves depend on some other unknown time-based async event, like fetching data before subscribing; the delay can't be set programmatically, but a dispose after first listen can.

rrousselGit commented 2 years ago

I agree with others that a temporary listener, or time based condition, feel awkward - what if the detailed components that subscribe to the provider themselves depend on some other unknown time-based async event, like fetching data before subscribing; the delay can't be set programmatically, but a dispose after first listen can.

You can close the temporary listener whenever you want.

Same goes for the initialization. You'll call ref.read when you want. If your onTap has other async functions, they'd likely execute before the ref.read – since the read will depend on them.

talent-apps commented 2 years ago

@rrousselGit The entire point of this feature is separation of concerns. Creating an ad-hoc listenter where it is not needed seems a bit weird.

rrousselGit commented 2 years ago

There's no reason for it to feel weird. It's a fairly common pattern for testing

final container = ProviderContainer();
final sub = container.listen(autoDisposeProvider, (prev, value) {});

expect(sub.read(), 42);

That's the expected way to "read" autoDispose providers.

talent-apps commented 2 years ago

Just for the sake of the example, if the navigation to the screen that uses that auto disposable provider is done in 10 different places, each one would have to be aware of this and create its own dummy listener, instead of encapsulating this behaviour in the provider itself.

rrousselGit commented 2 years ago

I already shared how to do that in the provider itself, using onCancel/invalidate.

To be clear, I've been thinking for a while about removing the ability to read autoDispose providers. That's ultimately the root of the issue.

There are ways to achieve the same thing without relying on ref.read(autoDispose)

rrousselGit commented 2 years ago

What I really want to avoid is adding an option flag.
Options tend to increase the complexity of the API quite a bit.

If this behavior is really desirable, I would prefer having a breaking change and making this the new behavior for all autoDispose providers. Having such behavior as a flag IMO is a bad idea

It's actually something I originally wanted to do for the 1.0.0 (cf https://github.com/rrousselGit/river_pod/issues/531).

robpot95 commented 2 years ago

I agree with option flag it can get narly, but i feel like it's described it should be the default behaviour of autoDispose

XuanTung95 commented 2 years ago

Example:

Some function on screen A initalizes a provider with some initial value and then navigates to screen B. Only screen B watches the provider. Once screen B is disposed, the provider should be disposed.

@talent-apps I wonder if this solution solves your problems?

talent-apps commented 2 years ago

@XuanTung95 Interesting idea, it's worth testing it.

rrousselGit commented 2 years ago

Might as well do what I suggested before

https://github.com/rrousselGit/river_pod/issues/1329#issuecomment-1085868131

robpot95 commented 2 years ago

Might as well do what I suggested before

#1329 (comment)

I've tested it out and seems not to work. The value is disposed when i switch to screen2 but is accessible when i go to screen3

final counterProvider = StateProvider.autoDispose<int>((ref) {
  ref.keepAlive();
  ref.onCancel(ref.invalidateSelf);
  return 0;
});

class Screen1 extends ConsumerWidget {
  const Screen1({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context, WidgetRef ref) {
    return Center(
      child: ElevatedButton(
        child: const Text("Go to screen2"),
        onPressed: () {
          ref.read(counterProvider.state).state = 10;
          Navigator.push(context, MaterialPageRoute(builder: (context) => const Screen2()));
        },
      ),
    );
  }
}

class Screen2 extends ConsumerWidget {
  const Screen2({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context, WidgetRef ref) {
    final counter = ref.watch(counterProvider);
    return Center(
      child: Column(
        mainAxisAlignment: MainAxisAlignment.center,
        children: [
          Text(counter.toString()),
          ElevatedButton(
            child: const Text("Go to screen3"),
            onPressed: () {
              ref.read(counterProvider.state).state = 10;
              Navigator.push(context, MaterialPageRoute(builder: (context) => const Screen3()));
            },
          ),
        ],
      ),
    );
  }
}

class Screen3 extends ConsumerWidget {
  const Screen3({Key? key}) : super(key: key);

  @override
  Widget build(BuildContext context, WidgetRef ref) {
    final counter = ref.watch(counterProvider);
    return Center(
      child: Text(counter.toString()),
    );
  }
}
rrousselGit commented 2 years ago

Well that's doing exactly what this issue is suggesting

This feature request would have the same problem.

rrousselGit commented 2 years ago

But to be fair, it sounds like you folks want cacheTime instead of disposeDelay

robpot95 commented 2 years ago

ref.read should maintain the state of auto-dispose providers. I dont see why it gets disposed when clearly listening to the provider on next screen. CacheTime is great, but I dont think it's the solution to this problem.

Setting a time may cause it to dispose to early or keep the state for to long.

rrousselGit commented 2 years ago

cacheTime is the expected solution. Destroying the value as soon as the user leaves the screen is rarely the expected UX.
Users typically would expect that quickly leaving and coming back to a screen bypasses the loading state.

If anything, cacheTime should default to a few minutes. That's how it is in other techs

XuanTung95 commented 2 years ago

But to be fair, it sounds like you folks want cacheTime instead of disposeDelay

@rrousselGit

robpot95 commented 2 years ago

cacheTime is the expected solution. Destroying the value as soon as the user leaves the screen is rarely the expected UX. Users typically would expect that quickly leaving and coming back to a screen bypasses the loading state.

If anything, cacheTime should default to a few minutes. That's how it is in other techs

Then you are stuck with a state for to long. E.g if you fetch news, switch screen and go back. I would like to refetch the news. By dispose the old one as soon i left. In this case i dont like to have magic numbers.

rrousselGit commented 2 years ago

Then you are stuck with a state for to long. E.g if you fetch news, switch screen and go back. I would like to refetch the news. By dispose the old one as soon i left. In this case i dont like to have magic numbers.

That's only temporary. There's a separate feature in progress that'll separate "call dispose" from "keep the value"

I'm thinking about giving more control to dispose a provider. Like, turn on/off autoDispose at run time or smth. So if provider is disposed at the wrong place, just tell people to turn on/off autoDispose at the right time. What do u think?

You already have all the control you need

there's ref.keepAlive/onCancel/onResume/invalidateSelf and cacheTime+disposeDelay, and also the the typical "listen + close subscription"

XuanTung95 commented 2 years ago

You already have all the control you need

there's ref.keepAlive/onCancel/onResume/invalidateSelf and cacheTime+disposeDelay, and also the the typical "listen + close subscription"

@rrousselGit

OK I think that will work. Just some comment.

rrousselGit commented 2 years ago
  • keepAlive is in AutoDisposeRef so I need to store ref for every provider in order to call keepAlive. Could you make it easier to use?

keepAlive is private to the provider.

If you want to maintain the state of a provider from outside the provider, use listen+close

  • If I have a normal provider or family which is not .autoDispose. Later I want to .autoDispose it, could I have a method dontKeepAlive? The workaround would be to create .autoDispose and set keepAlive then store the KeepAliveLink, then close() it, which is a lot of code.

I don't understand what you're trying to do. Could you share some code?

talent-apps commented 2 years ago

@rrousselGit Re: "Destroying the value as soon as the user leaves the screen is rarely the expected UX. Users typically would expect that quickly leaving and coming back to a screen bypasses the loading state."

Not sure why you assume this. The scenario that was presented earlier (list of items and a screen to edit an existing item) requires initialization of the provider every time rather than caching a previous value.

rrousselGit commented 2 years ago

Not sure why you assume this. The scenario that was presented earlier (list of items and a screen to edit an existing item) requires initialization of the provider every time rather than caching a previous value.

Re-initializing the provider when re-opening the screen and keeping the previous state are two different things

They aren't mutually exclusive, and you can do both at the same time. As I mentioned, there's a wip feature for this.

talent-apps commented 2 years ago

@rrousselGit The suggested solution of https://github.com/rrousselGit/river_pod/issues/1329#issuecomment-1085868131 doesn't seem to work. How should we implement it with the current lib?

rrousselGit commented 2 years ago

It should work. What do you mean by "doesn't work"?

talent-apps commented 2 years ago

Check out https://github.com/rrousselGit/river_pod/issues/1329#issuecomment-1086828636 "The value is disposed when i switch to screen2"

rrousselGit commented 2 years ago

That looks like a bug with StateProvider (& likely State/ChangeNotifierProvider)

Provider doesn't behave this way. Will fix