rrousselGit / riverpod

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

Warn against empy build methods #3031

Open rrousselGit opened 10 months ago

rrousselGit commented 10 months ago

Some folks do:

State? build() => null

This likely stems from a misusage of the Riverpod API and should therefore be discouraged

rrousselGit commented 10 months ago

This probably should happen only if build returns a Future/Stream

xleon commented 10 months ago

Hi, new to Riverpod here. This is my case:

@Riverpod(keepAlive: true)
class Package extends _$Package {
  @override
  PackageInfo? build() {
    return null;
  }

  Future<void> init() async => state = await PackageInfo.fromPlatform();
}

Why? Because I want to directly access the value rather than working with AsyncValue. Do you know a better way?

xleon commented 10 months ago

Ha!, found the answer to my question right in the docs: https://riverpod.dev/docs/concepts/scopes#initialization-of-synchronous-provider-for-async-apis

Reprevise commented 10 months ago

@rrousselGit Would this still be supported? This is a common pattern as specified in this article.

@riverpod
class SignInScreenController extends _$SignInScreenController {
  @override
  FutureOr<void> build() {
    // no-op
  }

  Future<void> signInAnonymously() async {
    final authRepository = ref.read(authRepositoryProvider);
    state = const AsyncLoading();
    state = await AsyncValue.guard(() => authRepository.signInAnonymously());
  }
}
rrousselGit commented 10 months ago

No. This is the very thing that we're looking to discourage. You're not supposed to do that.

Reprevise commented 10 months ago

No. This is the very thing that we're looking to discourage. You're not supposed to do that.

So currently what is the best way to do it then? Or does Riverpod just not support view controllers in that sense?

rrousselGit commented 10 months ago

How to do what exactly?

Reprevise commented 10 months ago

How to do what exactly?

This is currently the pattern that is used: image

If I wanted to do something like MVC with Riverpod, is that just not advised? Because using a no-op build method seems like the only way to accomplish this using the Notifier syntax. The only other way I can think of is to use ChangeNotifier, which doesn't even have riverpod_generator support.

I want to have a controller for each of my "view" widgets so that way the widget isn't directly calling a repository.

rrousselGit commented 10 months ago

I don't see the relationship between the graph shown and an empty build method.

Reprevise commented 10 months ago

I don't see the relationship between the graph shown and an empty build method.

The SignInController is an AsyncNotifier with a no-op build method. Since that is not advisable, the only other way that I think it can be done is via a ChangeNotifier, which has no riverpod_generator support.

AhmedLSayed9 commented 10 months ago

@Reprevise This should be covered by https://github.com/rrousselGit/riverpod/issues/1660.

Currently, I'm using Option from fpDart and use None to indicate this is an initial state. check this

You can also use some custom state instead of Option or an enum if your state has no data to be preserved, i.e:

enum SignInState {
  idle,
  success,
}

@riverpod
class SignInScreenController extends _$SignInScreenController {
  @override
  FutureOr<SignInState> build()=> SignInState.idle;

  Future<void> signInAnonymously() async {
    final authRepository = ref.read(authRepositoryProvider);
    state = const AsyncLoading();
    state = await AsyncValue.guard(() => authRepository.signInAnonymously().then((_) => SignInState.success););
  }
}
rrousselGit commented 10 months ago

I don't see the relationship between the graph shown and an empty build method.

The SignInController is an AsyncNotifier with a no-op build method. Since that is not advisable, the only other way that I think it can be done is via a ChangeNotifier, which has no riverpod_generator support.

Why?

I don't have a single clue about why SignInController has to have an empty build method or what this has to do with MVC or which problem it's trying to solve.

Please be more specific about the problem at hand. I do not understand at all what we're talking about here 😛

bizz84 commented 10 months ago

Since one of my articles was mentioned here, I may be able to provide some context.

Considering a similar example to the one above:

@riverpod
class AccountScreenController extends _$AccountScreenController {
  @override
  FutureOr<void> build() {
    // nothing to do
  }
  Future<void> signOut() async {
    final authRepository = ref.read(authRepositoryProvider);
    state = const AsyncLoading();
    state = await AsyncValue.guard(() => authRepository.signOut());
  }
}

And the corresponding widget:

/// This widget is only ever used if the user is already signed in
class AccountScreen extends ConsumerWidget {
  const AccountScreen({super.key});

  @override
  Widget build(BuildContext context, WidgetRef ref) {
    final state = ref.watch(accountScreenControllerProvider);
    // note: this widget only cares a about success/loading/error, but *not* the value of the stream
    return state.watch(...);
 }   

In this scenario, I think it makes sense for the controller's build method to return FutureOr<void> with an empty body, since I read the stream data using realtime updates somewhere else. Example:

@Riverpod(keepAlive: true)
Stream<AppUser?> authStateChanges(AuthStateChangesRef ref) {
  final authRepository = ref.watch(authRepositoryProvider);
  return authRepository.authStateChanges();
}

The way I've been using these APIs in this case, is that inside a button callback, I may do:

ref.read(accountScreenControllerProvider.notifier).signOut();

But the place where I listen to the auth state is completely separate - e.g. in the GoRouter declaration:

@Riverpod(keepAlive: true)
GoRouter goRouter(GoRouterRef ref) {
  final authRepository = ref.watch(authRepositoryProvider);
  return GoRouter(
    refreshListenable: GoRouterRefreshStream(authRepository.authStateChanges()),
    ...
  );
}

Especially when dealing with realtime updates, I find it reasonable to do a mutation using an AsyncNotifier<void> subclass like the one above, and for another (unrelated) widget to listen for updates.

Though someone on Discord made a very good point:

if the mutation results in altering the state of the provider, then the mutation itself should update the state, or invalidate it

And I agree this would be a good example:

@riverpod
class Something extends _$Something {
  FutureOr<List<SomeData>> build() => [];

  Future<void> deleteItem(SomeData item) async {
     final api = ref.read(apiProvider);
     await api.deleteData(id: item.id);
     await update((s) => s..remove(item));
  }

  Future<void> createItem(...) async {
     final api = ref.read(apiProvider);
     // doesnt return anything
     await api.createItem(...);
     ref.invalidateSelf(); // mark the cache as invalid
     // await future; // if you want this to only complete when the new value is aquired
  }

  Future<void> updateItem(SomeData item, SomeData Function(SomeData) updateFn) async {
     final api = ref.read(apiProvider);
     // doesnt return anything
     final updated = await api.replaceItem(id: item.id, newItem: updateFn(item));
     await update((current) async => current.map((e) {
          if (e.id == updated.id) return updated.data;
          return e;
     }).toList());
  }
}

I suppose something similar could be done when working with streams also, since we now have StreamNotifier:

@riverpod
class AuthStateController extends _$AuthStateController {
  @override
  Stream<AppUser?> build() {
    return ref.watch(authRepositoryProvider).authStateChanges();
  }

  Future<void> signInWithEmailAndPassword(String email, String password) async {
    final authRepository = ref.read(authRepositoryProvider);
    state = const AsyncLoading();
    await authRepository.signInWithEmailAndPassword(email, password);
    // provider will rebuild automatically once `authStateChanges` emits a new value
  }
  Future<void> signOut() async {
    final authRepository = ref.read(authRepositoryProvider);
    state = const AsyncLoading();
    await authRepository.signOut();
    // provider will rebuild automatically once `authStateChanges` emits a new value
  }
}

What's your take on this @rrousselGit?

Would my use case warrant the creation of AsyncNotifier<void> and empty build method?

Or should I redesign my AsyncNotifiers and StreamNotifiers to always load the data inside build?

If the widget that listens to said AsyncNotifier or StreamNotifier doesn't care about the data (since this is read somewhere else), isn't it redundant to force the notifier to load it?

neiljaywarner commented 10 months ago

@bizz84 @rrousselGit

neiljaywarner commented 10 months ago

@bizz84 @rrousselGit i know you're both working on it but i would especially love to see (or help build or see something close to) sort of a drag and drop robust solid pattern for an Async that a experienced flutter dev can make a sample of in a project for a brand new flutter dev (there usually are many and flutter usually tries to make it easy for them) that "just works"

Given a) do async code and show snackbar or modal dialog or go to another screen b) do async code and modify a list or similar It feels like 20-40% or more of our screens/features fit closer into a pattern a then b, and it would be nice to optimize for that (ie login, save to playlist, possible reply etc, possibly favorite etc)

https://riverpod.dev/docs/essentials/side_effects#going-further-showing-a-spinner--error-handling is great except 1) It doesn't seem (at least to me, i could be wrong) to fit A from above as well (yes, i know you could make an enum or sealed for Blah.success, Blah.falure, Blah.waiting but isn't that sort of duplicating asyncvalue? unless i'm confused 2) again, i could be wrong but for new to intermediate riverpod devs it feels like a "Selling point" /key point is to generally favor AsyncValue when() over FutureProvider and avoid StatefulWidgets generally. So far to me useBlahController and useState seems straightforward and simple and quick and easy but useFuture(possiblyuseMemoized) did not at least at first glance, but i did not come from react i just like simple hooks over stateful widget boilerplate

sorry for long comment hopeful more helpful than not

rrousselGit commented 10 months ago

Or should I redesign my AsyncNotifiers and StreamNotifiers to always load the data inside build?

Yes, they should always do so.
There are docs which showcase what I would recommend: https://riverpod.dev/docs/essentials/side_effects#going-further-showing-a-spinner--error-handling

To me an empty build method is a sign that maybe things that should be together currently aren't. Because if you have an "update" method, you must have a "get" one somehow. So that fetching should be done in build

rrousselGit commented 10 months ago

@neiljaywarner I think you're just looking for #1660

The pattern shown in that doc is only "In a world without mutations".

neiljaywarner commented 10 months ago

@rrousselGit thanks so much for your super prompt response, very cool. Yes, I'm following 1660 eagerly but patiently - except, i still do not see how this fits a paradigm like login use case where void seems appropriate bc AsyncError handlesa error and success just means go to next page, there is nothing to "update", there is no "get" yet it is still (I think) a mutation/side effect in the sense that it is POST not GET, not fetching data

l-7-l commented 6 months ago

Hi, new to Riverpod here. This is my case:

@Riverpod(keepAlive: true)
class Package extends _$Package {
  @override
  PackageInfo? build() {
    return null;
  }

  Future<void> init() async => state = await PackageInfo.fromPlatform();
}

Why? Because I want to directly access the value rather than working with AsyncValue. Do you know a better way?

same here. maybe it's worth considering allowing it if there are no side effects?

/// prodvider.dart
@Riverpod(keepAlive: true)
class ProductsNotifier extends _$ProductsNotifier {
  @override
  ProductsState build() => ProductsState.empty();

  Future<void> fetchOneProduct(ID id) async {
    state = state.copyWith(status: const AsyncStatus.loading());

    final res = await ref.read(productsRepositoryProvider).fetchOneProduct(id);

    state = state.copyWith(
      status: const AsyncStatus.success(),
      paramKeys: state.paramKeys.mergeList(res.paramKeys),
      paramValues: state.paramValues.mergeList(res.paramValues),
      products: state.products.add(res.product),
    );
  }
}
/// state.dart
@freezed
class ProductsState with _$ProductsState {
  const factory ProductsState({
    required Normalized<Product> products,
    required Normalized<ParamKey> paramKeys,
    required Normalized<ParamValue> paramValues,
    required IMap<ID, PricingSchema> pricingSchemas,
    required AsyncStatus status,
  }) = _ProductsState;

  factory ProductsState.empty() => ProductsState(
        products: Normalized.empty(),
        paramKeys: Normalized.empty(),
        paramValues: Normalized.empty(),
        pricingSchemas: const IMapConst({}),
        status: const AsyncStatus.idle(),
      );
}
/// call it before page navigation or somethings like useEffect();
onTap: () {
    ref.read(productsNotifierProvider.notifier).fetchOneProduct(id);
    AutoRouter.of(context).push(ProductRoute(cover: cover, id: id));
},