Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mounted property on Notifier/AsyncNotifier #1879

Closed
mattermoran opened this issue Nov 4, 2022 · 13 comments
Closed

mounted property on Notifier/AsyncNotifier #1879

mattermoran opened this issue Nov 4, 2022 · 13 comments
Labels
question Further information is requested

Comments

@mattermoran
Copy link

While migrating from StateNotifier to a Notifier I noticed that the mounted property does not exist.
I can get around by tracking it manually but I just wonder if it will be added in the future or was removed on purpose because of reasons. Here's what I'm using at the moment

@riverpod
class MyNotifier extends _$MyNotifier {  
  @override
  void build() {
    mounted = true;
    ref.onDispose(() => mounted = false);
  }
  
  bool mounted = true;
}
@mattermoran mattermoran added enhancement New feature or request needs triage labels Nov 4, 2022
@rrousselGit
Copy link
Owner

There is no plan to add a mounted property due to its misleading nature.
The new Notifier and StateNotiifer have different disposal cycles. A StateNotifier is recreated when ref.watch changes, but a Notifier isn't.
So mounted would almost always be true in Notifier

Also, for that reason, I'd suggest against using a bool if you want such a property. Instead consider an object:

  @override
  void build() {
    key = Object();
    ref.onDispose(() => key = null);
  }

  void fn() async {
    final key = this.key;
   await something():
    if (key == this.key) {
      // still mounted
    }
  }

Also, you have direct access to Ref. So you can directly call onDispose, which is much better overall :)

@rrousselGit rrousselGit added question Further information is requested and removed enhancement New feature or request needs triage labels Nov 4, 2022
@mattermoran
Copy link
Author

I see.

So mounted would almost always be true in Notifier

Well I guess that's technically what I want. Just because it rebuilds doesn't mean it's not mounted. If it's AutoDispose type then I assume it will be disposed when it's not listened/watched anymore. But I guess with Notifier it can also be disposed when something it listens to changes. Trying to wrap my head around it. :)

Also, for that reason, I'd suggest against using a bool if you want such a property. Instead consider an object:

So in your example if notifier rebuilds during await then it will not be considered mounted as Object() != Object(). But I don't want that as technically it is still being listened to. Unless key == null in that case it's actually fully disposed

Also, you have direct access to Ref. So you can directly call onDispose, which is much better overall :)

Well there's no mounted or disposed property on it so I can't really use it after the await something()

@mattermoran
Copy link
Author

Anyway, what you're suggesting should do the trick. I realize I was trying to use the Notifier for things it was not meant to be used just because I like the fact that I can just annotate class with @riverpod and not create a class and a provider separately.

@rrousselGit
Copy link
Owner

Closing. As mentionned I have no plan on adding such property for now.

I understand the appeal, but I think the damage it could cause would be worse than the current inconvenience.
I believe #1660 should help here though, as we'll likely get a mutation-specific "ref", with its own "dispose".

@bizz84
Copy link
Contributor

bizz84 commented Feb 16, 2023

I've been trying to use the approach above and ended up with something like this:

class SomeNotifier extends AutoDisposeAsyncNotifier<void> {
  late Object? key; // 1. create a key

  @override
  void build() {
    key = Object(); // 2. initialize it
    ref.onDispose(() => key = null); // 3. set to null on dispose
  }

  void fn() async {
    final key = this.key; // 4. grab the key before doing any async work
    await someAsyncWork():
    if (key == this.key) { // 5. Check if the key is still the same
      // still mounted
    }
  }
}

Repeating the five steps above for every notifier is quite error prone.

So I attempted to move some of the logic into a mixin:

mixin NotifierMounted {
  Object? _initial;
  late Object _current;

  // Call this from the `build()` method
  void mount(Ref ref) {
    _current = Object();
    _initial ??= _current;
    // on dispose, set to a different [Object]
    ref.onDispose(() => _current = Object());
  }

  // Whether the notifier is currently mounted
  // This relies on the fact that an [Object] instance is equal to itself only.
  bool get mounted => _current == _initial;
}

This relies on the fact that _initial is only set once, while _current is set on every rebuild.

And allows me to expose a mounted getter that is easier to use than messing about with objects.

As a result, I ended up with 3 steps (instead of 5):

class SomeNotifier extends AutoDisposeAsyncNotifier<void>
    with NotifierMounted { // 1. add mixin

  @override
  void build() {
    mount(ref); // 2. register the key
  }

  void fn() async {
    await someAsyncWork();
    if (mounted) { // 3. Check if still mounted
      // still mounted
    }
  }
}

Would this be an acceptable implementation?

@rrousselGit
Copy link
Owner

No, because you circled back to a bool.

The whole point is that a bool is unacceptable. You cannot have if (mounted) match if (key == this.key)

@bizz84
Copy link
Contributor

bizz84 commented Feb 16, 2023

I guess I don't fully understand this yet.

My impression is that key == this.key will be true if the provider didn't rebuild or got disposed while the async operation was in progress.

Likewise, I've defined my mounted getter as:

bool get mounted => _current == _initial;

which also will be true if the provider didn't rebuild or got disposed (if either of these things happened, _current would have been set to a new value).

Regardless, my main concern is to know if the provider was disposed (I don't care if it rebuilds but is still alive).

And for that, even the original bool mounted flag solution seems to work well in all my tests.


Coming back to your original comment:

So mounted would almost always be true in Notifier

Given that I'm using this only with AutoDispose notifiers, and all I need is to know if they are disposed, would it make sense to have my own bool disposed property, that mimics what the old StateNotifier was doing?

mixin NotifierDisposed {
  bool _disposed = false;

  void setDisposed() => _disposed = true;

  bool get disposed => _disposed;
}

class SomeNotifier2 extends AutoDisposeAsyncNotifier<void> with NotifierDisposed {

  @override
  void build() {
    ref.onDispose(setDisposed);
  }

  void fn() async {
    await someAsyncWork();
    if (!disposed) {
      // still mounted
    }
  }
}

@rrousselGit
Copy link
Owner

rrousselGit commented Feb 16, 2023

Again, no. If that worked then it'd be built-in.

The core idea is that fn should get canceled if build was re-executed while fn is pending.
That's how StateNotifier behaves as the StateNotifier is disposed on ref.watch. But that's not how Notifier works.

Anything based on a boolean will not handle this

@bizz84
Copy link
Contributor

bizz84 commented Feb 16, 2023

The core idea is that fn should get canceled

Cancelling a network operation is not always the desired behaviour though.

Sometimes you want the operation to complete regardless, but still allow for the widget to be dismissed (and the corresponding Notifier to be disposed).

In that case, the Object based approach would work. But I was just pointing out that it requires a lot of boilerplate:

class SomeNotifier extends AutoDisposeAsyncNotifier<void> {
  late Object? key; // 1. create a key

  @override
  void build() {
    key = Object(); // 2. initialize it
    ref.onDispose(() => key = null); // 3. set to null on dispose
  }

  void fn() async {
    final key = this.key; // 4. grab the key before doing any async work
    await someAsyncWork():
    if (key == this.key) { // 5. Check if the key is still the same
      // still mounted
    }
  }
}

Ideally, there should an easier way to handle this.

@ggirotto
Copy link

I ended up subclassing AutoDisposeAsyncNotifier to share this behavior across all providers that I have in the app and avoid the need of recreating this manual check of keys on each one of them. The solution is not ideal, but since it was said that there's no plan to add mounted property to the providers it's the best way to center this behavior somewhere instead polluting all view models with this.

For those who have interest, here's the solution I ended up with:

abstract class CustomAutoDisposeAsyncNotifier<T> extends AutoDisposeAsyncNotifier<T> {
  Object? key;

  @override
  FutureOr<T> build() async {
    key = Object();
    ref.onDispose(onDispose);
    final result = await init();
    return result;
  }

  Future<void> guard(
    Future<T> Function() future, {
    FutureOr<void> Function(Object error, StackTrace stackTrace)? onError,
  }) async {
    try {
      final key = this.key;
      final result = await future();
      if (key == this.key) state = AsyncValue.data(result);
    } catch (err, stack) {
      if (key != this.key) return;

      if (onError != null)
        onError(err, stack);
      else
        state = AsyncValue.error(err, stack);
    }
  }

  FutureOr<T> init();

  void onDispose() {
    key = null;
  }
}

Here's an usage example:

final provider = AsyncNotifierProvider.autoDispose<ProviderImpl, ProviderState>(
  CheckoutVM.new,
  dependencies: [....],
  name: 'provider',
);

class ProviderImpl extends CustomAutoDisposeAsyncNotifier<ProviderState> {
  @override
  FutureOr<ProviderState> init() => const ProviderState();

  Future<void> asyncFunction() async {
    state = const AsyncValue.loading();
    await guard(
      () async {
        final result = await asyncFunction();
        return ProviderState(result: result);
      },
    );
  }

  void onDispose() {
    // Use this instead `ref.onDispose`
  }
}

The custom guard function replaces state = AsyncValue.guard(...) by adding this mounting check security.
Key painful points:

  • Instead of overriding build function, the subclasses will need to override init (not that big deal, just need to pay attention to don't implement the wrong function)
  • ref.onDispose shouldn't be used. Instead override onDispose function, which will be invoked by BSAutoDisposeAsyncNotifier when provider gets disposed.

Again, not ideal solution but for my use-case is much less painful to subclass the original provider than implementing a request cancelation token strategy that needs to be passed alongside all layers of the application.

@iamnabink
Copy link

@ggirotto thanks for this, you rock 🚀

@agordeev
Copy link

agordeev commented Aug 20, 2024

The core idea is that fn should get canceled if build was re-executed while fn is pending.

What is the proposed solution for this problem then? We have a whole section docs about side effects, and this issue is reproducible with those code examples if you leave a page that uses that Notifier.

Edit: it seems that @rrousselGit suggests using ref.onDispose and object comparison to determine if a notifier was disposed. Is there any way to include that into the framework itself to avoid pretty much of the boilerplate code?

@rrousselGit
Copy link
Owner

A ref.mounted has been added on the 3.0 branch already.
3.0 has some breaking changes to make this possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

6 participants