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

Reduce boiler plate code for "mounted" in the new AsyncNotifier class. #1926

Closed
iLoveDocs opened this issue Nov 19, 2022 · 5 comments
Closed
Labels
enhancement New feature or request needs triage

Comments

@iLoveDocs
Copy link

When using AsyncNotifier, I always need to write 4 lines of boilerplate code just to check for mounted before updating the state.

@riverpod
class Foo extends AutoDisposeAsyncNotifier<void> {
  final Object _original = Object(); // #1
  late Object? _current = _original; // #2
  bool get mounted => _original == _current; // #3

  @override
  FutureOr<void> build() {
    ref.onDispose(() => _current = null); // #4
    return null;
  }

  Future<void> someFunc() async {
    state = AsyncLoading();
    final newState = await AsyncValue.guard(() => anAsyncFunc());
    if (mounted) state = newState;
  }
}

Describe the solution you'd like

I think mounted property should be added to AsyncNotifier in some way. Maybe add a Riverpod(...) annotation to silently ignore that error.

@iLoveDocs iLoveDocs added enhancement New feature or request needs triage labels Nov 19, 2022
@rrousselGit
Copy link
Owner

There is voluntarily no mounted property, because it is unreliable in the context of Notifier/AsyncNotifier.

Building your own isn't what you're supposed to do. Instead cancel your async operations on dispose.

@iLoveDocs
Copy link
Author

@rrousselGit

Yes, before opening this issue, I read about it. But you don't always have something you can cancel, for example, in the code I shared, how will I cancel the future anAsyncFunc? Had it been a Timer or an http request, or something that could be cancelled, then yes, onDispose is the place to handle that, but in most cases we don't have those types that can be cancelled (or at least I don't know how to cancel them without using CancelableCompleter type thing -- again more boilerplate code)

@rrousselGit
Copy link
Owner

It's up to your async functions to expose ways to cancel them. CancelableCompleter is one way. Dio's CancelToken is another.
There's not really a finite solution here

The point is, a mounted property doesn't fit well Notifier.

In the end what you truly want is #1660 which is a broader issue which in part would solve your problem

@otto-dev
Copy link

otto-dev commented Dec 2, 2022

@iLoveDocs I don't understand what those object equality checks are for, why don't you just

class Foo extends AutoDisposeAsyncNotifier<void> {
  bool _mounted = false;

  @override
  FutureOr<void> build() {
    _mounted = true;
    ref.onDispose(() => _mounted = false);
  }
  // ...
}

@rrousselGit

The async operation implementation is not always under your control, and using CancelableCompleter instead of something like .isDisposed can lead to unnecessary boilerplate. Having a .isClosed property is common practice for closable (here, disposable) resources.

Is there a technical reason why .isDisposed is not provided, or is it a deliberate design choice?

@rrousselGit
Copy link
Owner

I don't understand what those object equality checks are for, why don't you just

Async operations should probably be canceled on ref.watch change. A boolean wouldn't handle this. An object would. But this approach is not one that can easily be "build-i"

The current situation isn't ideal. But as I said before, that's in the scope of #1660 to fix

This probably should be closed in favor of #1660

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs triage
Projects
None yet
Development

No branches or pull requests

3 participants