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

NotifierProvider misses ref #2460

Closed
passsy opened this issue Apr 12, 2023 · 9 comments
Closed

NotifierProvider misses ref #2460

passsy opened this issue Apr 12, 2023 · 9 comments
Assignees
Labels
documentation Improvements or additions to documentation

Comments

@passsy
Copy link
Contributor

passsy commented Apr 12, 2023

I noticed an API difference between Provider, ChangeNotifierProvider and NotifierProvider. NotifierProvider doesn't take ref as argument in NotifierProvider.creator and NotifierProvider.overrideWith.

All I really want is to define a NotifierProvider<NotifierT, T> and swap the actual implementation of that provider at runtime, making all Providers that depend on NotifierT and T rebuild.

And that already works for Provider<T>!

final featureFlag = StateProvider<bool>((ref) => false);
// Works but doesn't notify when Counter changes
final count = Provider<Counter>((ref) {
  final flag = ref.watch(featureFlag);
  if (flag) {
    return AlwaysFive();
  }
  return CounterImpl();
});

It works with ChangeNotifierProvider<T>!

final featureFlag = StateProvider<bool>((ref) => false);
final count = ChangeNotifierProvider<Counter>((ref) {
  final flag = ref.watch(featureFlag);
  if (flag) {
    return AlwaysFive();
  }
  return CounterImpl();
});

But it does not work for NotifierProvider<NotifierT, T> because NotifierProvider.creator doesn't pass ref as argument. I do not understand the rational between the APIs being different.

final featureFlag = StateProvider<bool>((ref) => false);
// Can't subscribe to featureFlag
final notifierCount = NotifierProvider<Counter, int>(() {
  // ERROR: no ref
  final flag = ref.watch(featureFlag);
  if (flag) {
    return AlwaysFive();
  }
  return CounterImpl();
});

Same for overrideWith which takes ref as argument for Provider and ChangeNotifierProvider but not for NotifierProvider.

Is this an oversight, intentional or just a limitation of the current implementation?


This is a follow-up to #2458

@passsy passsy added documentation Improvements or additions to documentation needs triage labels Apr 12, 2023
@rrousselGit
Copy link
Owner

This is intentional.

It doesn't make sense for two reasons:

  • The Notifier instance is preserved during ref.watch updates.
    A "watch" dependency change does not recreate the Notifier; it only re-executes build.
  • Your proposal is incompatible with code-generation, which will be the default syntax once we have metaprogramming (and doing final provider = MyProvider(...) will be deprecated)

@passsy
Copy link
Contributor Author

passsy commented Apr 13, 2023

That's fine but it should be documented.

Neither the documentation of Notifer, NotifierProvider nor the landing PR states that NotifierProvider can only ever create a single instance per Container, that is cached throughout the lifetime of the Container.

When I discovered Notifier I saw it as a pure Dart replacement for ChangeNotifier or a pure riverpod alternative to StateNotifier. That there might be a different lifecycle for Notifier and StateNotifier is counter-intuitive to me.


Your proposal is incompatible with code-generation, which will be the default syntax once we have metaprogramming

Can't follow you here. The syntax of the ChangeNotifierProvider example is valid today.
My proposal is to make the NotifierProvider identical to the syntax of ChangeNotifierProvider. How can that be incompatible?

@rrousselGit
Copy link
Owner

It already is documented that the Notifier instance is preserved across ref.watch updates. Cf the dartdoc of the build method:

On the other hand, the AsyncNotifier will not be recreated. Its instance will be preserved between executions of build.

https://pub.dev/documentation/riverpod/latest/riverpod/AsyncNotifier/build.html

@rrousselGit
Copy link
Owner

Can't follow you here. The syntax of the ChangeNotifierProvider example is valid today.
My proposal is to make the NotifierProvider identical to the syntax of ChangeNotifierProvider. How can that be incompatible?

The syntax is:

@riverpod
class MyNotifier extends _$MyNotifier {
  @override
  int build() {
   <use ref here>
  }
}

There's no place for using ref to replace the Notifier instance

@passsy
Copy link
Contributor Author

passsy commented Apr 13, 2023

The documentation states:

If a dependency of this Notifier (when using Ref.watch) changes, then build will be re-executed. On the other hand, the Notifier will not be recreated. Its instance will be preserved between executions of build.

It explicitly states what happens when a dependency of Notifier changes, meaning watch is being called inside build().
(My code doesn't call watch inside Notifier.build().) I keep the same instance. All clear

I want to call watch in NotifierProvider.create which is not the dependency of the Notifier. (Because the Notifier doesn't know about the implementation of the NotifierProvider).

watch in NotifierProvider.create is the dependency of the NotifierProvider. To me those are two different things and the latter (my case) is not documented.

final notifierCount = NotifierProvider<Counter, int>((ref) {
  // Changes of featureFlag causes the lambda to run again
  final flag = ref.watch(featureFlag);
  if (flag) {
    return AlwaysFive();
  }
  return CounterImpl();
});
class Counter extends Notifier<int> {
  @override
  int build() {
    // doesn't call watch
    return 0;
  }

  void increment() {
    state++;
  }
}

class AlwaysFive extends Notifier<int> {
  @override
  int build() {
    // doesn't call watch
    return 5;
  }
}

Maybe there are not two different kind of dependencies in the current implementation - one for the NotifierProvider and one of the actual Notifier - but wouldn't that make sense?

@rrousselGit
Copy link
Owner

I want to call watch in NotifierProvider.create which is not the dependency of the Notifier. (Because the Notifier doesn't know about the implementation of the NotifierProvider).

  • There's no place where people specify a NotifierProvider.create in the annotation syntax
  • ref.watch behaves the same no matter where it is. It shouldn't have a different behavior is used within build vs used within the init function of providers.

@rrousselGit
Copy link
Owner

Maybe there are not two different kind of dependencies in the current implementation - one for the NotifierProvider and one of the actual Notifier - but wouldn't that make sense?

It makes sense but at the same time that would mean we'd effectively create two separate providers.

This can get a bit weird, as the notifier vs the "build" method may have different dependencies/life-cycles/...

@rrousselGit rrousselGit self-assigned this May 10, 2023
@zgramming
Copy link

Hello @rrousselGit , sorry for late discussion but i want make sure about using NotifierProvider instead of StateNotifierProvider because in documentation is recommended using NotifierProvider.

So i have simple MVVM architecture and have injection file to connected all provider, current implementation work out the box using StateNotifierProvider. But in last update package, i notice have new provider NotifierProvider and i want try migrate to it.

But i notice, NotifierProvider doesn't have ref and i can't migrate to this provider because this limitation. So i want make sure in the future NotifierProvider will have ref argument or not, if not i can keep implementation using StateNotifierProvider.

This some example about my code :

injection.dart

final smsNotifier = StateNotifierProvider<SMSNotifier, SMSState>(
    (ref) => SMSNotifier(repository: ref.watch(_smsRepository)));

final _smsRepository = Provider(
    (ref) => SMSRepository(localDatasource: ref.watch(_smsLocalDatasource)));

final _smsLocalDatasource =
    Provider((ref) => SMSLocalDatasource(box: ref.watch(_smsBox)));

final telephony = Provider((ref) => Telephony.instance);
final _smsBox = Provider((ref) => Hive.box<SMSModel>(hiveSMSBox));

exampleProvider.dart


class SMSLocalDatasource {
  final Box<SMSModel> box;
  SMSLocalDatasource({
    required this.box,
  });

  Future<String> insert(SMSModel model) async {
    await box.put(model.id, model);

    return "Berhasil menyimpan SMS";
  }

  Future<String> delete(String id) async {
    await box.delete(id);

    return "Berhasil menghapus SMS";
  }

  List<SMSModel> getAll() {
    return box.values.toList();
  }
}

class SMSRepository {
  final SMSLocalDatasource localDatasource;
  const SMSRepository({
    required this.localDatasource,
  });

  Future<Either<Failure, String>> insert(SMSModel model) async {
    try {
      final result = await localDatasource.insert(model);

      return Right(result);
    } catch (e) {
      return Left(CommonFailure(e.toString()));
    }
  }

  Future<Either<Failure, String>> delete(String id) async {
    try {
      final result = await localDatasource.delete(id);

      return Right(result);
    } catch (e) {
      return Left(CommonFailure(e.toString()));
    }
  }

  List<SMSModel> getAll() {
    return localDatasource.getAll();
  }
}

class SMSState extends Equatable {
  const SMSState({
    this.items = const [],
    this.onInsert = const AsyncData(null),
    this.onDelete = const AsyncData(null),
  });

  final List<SMSModel> items;
  final AsyncValue<String?> onInsert;
  final AsyncValue<String?> onDelete;

  @override
  List<Object> get props => [items, onInsert, onDelete];

  @override
  bool get stringify => true;

  SMSState copyWith({
    List<SMSModel>? items,
    AsyncValue<String?>? onInsert,
    AsyncValue<String?>? onDelete,
  }) {
    return SMSState(
      items: items ?? this.items,
      onInsert: onInsert ?? this.onInsert,
      onDelete: onDelete ?? this.onDelete,
    );
  }
}

class SMSNotifier extends StateNotifier<SMSState> {
  final SMSRepository repository;
  SMSNotifier({
    required this.repository,
  }) : super(const SMSState()) {
    getAll();
  }

  Future<void> insert(SMSModel model) async {
    final result = await repository.insert(model);

    result.fold(
      (l) => state =
          state.copyWith(onInsert: AsyncError(l.message, StackTrace.current)),
      (r) => state = state.copyWith(
        onInsert: AsyncData(r),
        items: [...state.items, model],
      ),
    );
  }

  Future<void> delete(String id) async {
    final result = await repository.delete(id);

    result.fold(
      (l) => state = state.copyWith(
        onDelete: AsyncError(l.message, StackTrace.current),
      ),
      (r) => state = state.copyWith(
        onDelete: AsyncData(r),
        items: state.items.where((e) => e.id != id).toList(),
      ),
    );
  }

  void getAll() {
    state = state.copyWith(items: repository.getAll());
  }
}

@rrousselGit
Copy link
Owner

Work tracked in #2630

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants