rrousselGit / riverpod

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

Why not call read in the body of the provider? #864

Closed esDotDev closed 2 years ago

esDotDev commented 3 years ago

I'm confused about this line the docs: image

Could you elaborate on the rationale?

Why is this good:

final repositoryProvider = Provider((ref) => Repository(ref.read));
class Repository {
   Repository(this.read);
   final Reader read;
   String get token => read(userTokenProvider).state;
}

But this is bad?

final repositoryProvider = Provider((ref) => Repository(ref.read(userTokenProvider));
class Repository {
  Repository(this.token);
   final UserToken token;
   String get token => userToken.state;
}

It seems from the Repository standpoint it's cleaner to say the thing it wants, rather than concern itself with how that thing is injected. Especially as read opens the Repository full access to all other providers, while passing UserToken tells it only what it needs to know.

I actually like it even dumber, where it only takes a string, which removes another dependency from the Repository that it need not know about (ie UserToken):

final repositoryProvider = Provider((ref) => Repository(() => ref.read(userTokenProvider).state);
class Repository {
  Repository(this.tokenDelegate);
   final String Function() tokenDelegate;
   String get token => tokenDelegate();
}

This ends up with the most portable code, as this repository is now unconcerned with the DataInjection layer and just needs a simple string builder.

esDotDev commented 3 years ago

I thought maybe the issue was that read is only called once in the latter 2 examples, but is called repeatedly in the first. But in my case, we're passing ChangeNotifier/StateNotifier objects, so the instance of the thing is never going to change.

With the closure method, we call read each time we lookup the token as well, so it seems identical in behavior, without injecting the riverpod classes into the repo itself.

rrousselGit commented 3 years ago

There is an explanation in the documentation: https://riverpod.dev/docs/concepts/reading#using-refread-to-obtain-the-state-of-a-provider-once

Is it not good enough?

rrousselGit commented 3 years ago

As for getters that read providers, this isn't a good practice either. At the very least, those getters should be private. And you should guarantee that they aren't called during the initialization of your provider

As for abstracting ref, this is unnecessary. Such abstraction should not be needed, and doing so makes your application more complex for a very small gain.

Trying to optimize your code for "I want Riverpod to be easily removable" seems counter-productive. From experience, this causes more harm than good.

esDotDev commented 3 years ago

Ah I see, the rationale is on a different page from the big red box, which is on the "Combining providers" page. A link here might help.

Thanks for the clarification, the rationale seems to be a future proofing one, "well the ref might change in the future, even if you know it will not right now". I appreciate that argument, but also it doesn't really seem important when we're talking about singleton like instances like an auth-service, but point taken, it does leave the door open for issues.

Regarding last points. It's not so much designing riverpod to be removable, as it is designing around some first principles, of don't inject dependencies into a class that it doesn't need.

Giving every service access to every provider in the app seems like a much nastier source of potential bugs than worry about one day a singleton might change. Don't you think? In one case the service is limited to only it's role, it has no ability to do anything outside of itself. In the other, services could easily warp into controllers, and on a large team things could get messy fast.

Finally, in my 3rd snippet, I believe this is already future-proofed anyways, as it does read the provider each time it needs a username, so it would work despite the singleton changing. We only capture the ref in the closure, not the provided instance:

final repositoryProvider = Provider((ref) => Repository(() => ref.read(userTokenProvider).state);
class Repository {
  Repository(this.tokenDelegate);
   final String Function() tokenDelegate;
   String get token => tokenDelegate();
}

Is that correct? This sort of on-demand read seems to be a good combination of robustness without injecting the whole world into the service.

I'm happy for controllers to have ref and be able to easily access eachother. It just seems that extending that functionality to the Service layer is an anti-pattern as it begins to blur lines and open up much more surface for bugs.

Happy to close this now, but I am interested in your thoughts on the above. Tell me why I'm wrong??

TimWhiting commented 3 years ago

In your case you aren't using ref.read in the body of the provider create function (since it is in the closure function instead). So it is valid solution like you mention to avoid injecting the whole ref world into the service & avoid ref.read in the create function.

Just curious why you aren't doing this?

final repositoryProvider = Provider((ref) => Repository(ref.watch(userTokenProvider).state));

class Repository {
   final String token;
   Repository(this.token);
}

This way you are not injecting the whole world of ref into the service, and you get the most recent token. This breaks down if Repository is not stateless, but usually the service layer should be stateless.

If the Repository has to have some local state and not rebuild this is also an alternative:

final repository = Repository();
  ref.listen<StateController<String>>(userTokenProvider, (last, controller) {
    repository.updateToken(controller.state);
  }, fireImmediately: true);
  return repository;
});

class Repository {
  Repository();
  String token = '';
  void updateToken(String token) {
    this.token = token;
  }
}
esDotDev commented 3 years ago

Yep that's a good point, if it's stateless we could just allow it to get re-created all the time and just use constructor injection. I guess I was trying to avoid that, but as you say, it shouldn't really matter.

And ya, I also thought of the .listen method that could continually inject the latest token, but just seemed like inline closure was kinda the more elegant/cleaner/simpler way. Readability wise, I think it's a clear win.

esDotDev commented 3 years ago

@TimWhiting I think I remembered why I didn't want to re-create the service.

If I have something like this:

static final todoModel = ChangeNotifierProvider((ref) {
  // Inject the todo service into the model
  TodoService service = ref.watch(todoService);
  return TodoModel(service);
});

Then if the service is re-created everytime the user changes, my model would get rebuilt, and I'd lose all my state.

I guess the 'solution' here is to not show any dependancies for TodoModel, and instead just give it the entire ref, but I dunno, it feels like using read is the lesser of two evils here, and from a high level declarative standpoint, passing the dep directly is definitely more readable/grokable, ie, this return TodoModel(service); is objectively better than return TodoModel(ref); from a self-documenting code standpoint.

TimWhiting commented 3 years ago

Well if the user changes and the service changes, don't you want to reset the model? After all it is a different user now.

I've definitely had times where I think that I need to use read in the body of a provider, and then realized that I've just structured my code poorly. Not saying that there isn't any valid cases where read might be the only option, but just that I've often found that its a question of state organization rather than that it needs to be done a certain way. ref.listen is one new addition that I think will help in the cases where I used to always need to pass ref to my Notifiers. But again, I don't know all of the problems you might run into, and every problem is different and requires different solutions. In general I treat the advice to not use read in the body of a provider as an indication of something that could be structured better, but I don't avoid it for getting something to initially work. (It doesn't actively harm your code, but can make it less flexible in the long run). That being said I almost always pass ref to my Notifiers, it is the most ergonomic and 'riverpod' way to do things.

rrousselGit commented 3 years ago

I think you're worrying too much about it. The ref object is designed to be passed to your model/service. Passing it is not an issue in any way

Bear in mind that more often than not, the exposed values and the providers are one and the same. We're not really using one without the other.
The fact that they are separated is only because of language/syntax limitations

esDotDev commented 3 years ago

Ya I am probably over-thinking it, like the entire widget tree has access to ref so I mean... does it really matter?

It's just, I do like declaring which dependencies controllers/services have on each-other, rather than it just being a total free-for-all.

Thanks for the chat guys! I'm not totally convinced that read is "bad practice", but I do understand the common error case you're trying to avoid by declaring it so. For on-demand look ups, like button handlers, or getMeTheCurrentUserName calls it seems like a solid approach.

esDotDev commented 3 years ago

I think you're worrying too much about it. The ref object is designed to be passed to your model/service. Passing it is not an issue in any way

It is a bit of an issue in the sense that someone reading the provider for AppModel has no clue what it's dependencies are. They are hidden within AppModel, and only with a careful read-through of the entire class can you be sure you've identified all the deps.

This is the same problem that, for years, has been leveled against singletons, they "hide dependencies".

I don't think it's a huge deal, the benefit of DI is worth it, but I don't think you can completely hand-wave it away. There is something fundamentally better about declaring clear interfaces and dependencies up front, if it's at all feasible (it's not in the widget tree).

TimWhiting commented 3 years ago

You might be interested in the dependencies parameter:

final provider = Provider((ref) => MyNotifier(ref, ref.watch(other)), dependencies: [other]);

This will automatically scope provider if other is scoped, and if anything else is watched or read in MyNotifier it will throw an Error. Remi can correct me if I'm wrong. There are some edge cases around using this parameter and overrideWithProvider though. The behavior and edge cases probably ought to be documented though.

rrousselGit commented 3 years ago

If it's just about getting the list of dependencies, Riverpod resolution is statically analyzable.

We could have a tool that analyzes the code of your app and generate a graph of the provider<>widget and provider<>provider interactions

jpcodr commented 3 years ago

Hi guys.

I have a repository and I want to expose one of its methods to get external data.

final repositoryProvider = Provider<Repository>((ref) => RepositoryImpl(ref));
final postsProvider = FutureProvider((ref) => ref.read(repositoryProvider).fetchPostList());

So this is a bad practice and i should change it to use watch instead?

final repositoryProvider = Provider<Repository>((ref) => RepositoryImpl(ref));
final postsProvider = FutureProvider((ref) => ref.watch(repositoryProvider).fetchPostList());
rrousselGit commented 3 years ago

yes, that is correct.

esDotDev commented 2 years ago

Came across this article today, it more fully explains why some people view this as an anti-pattern: https://blog.ploeh.dk/2010/02/03/ServiceLocatorisanAnti-Pattern/

In short:

the problem with Service Locator is that it hides a class' dependencies, causing run-time errors instead of compile-time errors, as well as making the code more difficult to maintain because it becomes unclear when you would be introducing a breaking change.

Not gospel or anything, but makes some good points about the benefits of defining dependencies in the constructor when possible.

marinkobabic commented 2 years ago

In order to build a loosely coupled applications only the required services should be passed in as parameter. Passing in ref means passing in the whole di container. This is just a fact even if many static analyzer can build a dependency tree. Passing in the service instead of ref makes riverpod replaceable by any other framework and this is the prove of loosely coupling already.

This does not necessarily mean that read method is the solution but I'm convinced that @rrousselGit will find a way to solve the technical issue.

esDotDev commented 2 years ago

Ya the more I think about it, the more this seems best-practice, not bad.

The fundamental use case is:

  1. I have Thing1, and it uses Thing2
  2. Thing1 is mutable, and does not want to rebuild whenever Thing2 changes.

The guidance is, "pass in read", and this is ok, but it's not great.

We could have it so dependencies are declared more explicitly:

class AppManager with ChangeNotifier {
  AppManager(this.getFileService) {
    getFileService().loadFile('app.sav').then(...);
  }
  final FileService Function() getFileService;
}

Is this not nicer from a testing standpoint? Deps are clear and obvious from the outside. And as mentioned above, code is more portable between SM solutions, which is a small thing, but nice to have if you can get it.

rrousselGit commented 2 years ago

The thing is, if you're using Riverpod, you're no-longer supposed to instantiate the class yourself.
So the constructor litteraly doesn't matter.

The present syntax is only because Metaprogramming doesn't exist yet.
Once it's there, providers and notifiers will fuse into a single entity with no constructor and a "ref" property.

Passing the ref is more future proof. It'll allow the migration tool to automatically use the new syntax for you

reedom commented 2 years ago

Hi guys, today I got a mention that the new document encourages to use watch over read as much as possible, while our code uses read a lot in providers and notifiers in DI manner. Prior to v1 we had taught that we shouldn’t pass watch to model/service. And I think that is still true today.

And @rrousselGit wrote in this thread:

I think you’re worrying too much about it. The ref object is designed to be passed to your model/service. Passing it is not an issue in any way

For instance, if I call watch in a stateful object, it ends up destroying the caller object itself, that usually is an unwanted behavior for stateful ones.

final repositoryProvider = StateNotifierProvider((ref) => Repository(ref));

final periodicProvider =
    StreamProvider((ref) => Stream.periodic(const Duration(seconds: 1)));

class Repository extends StateNotifier<String> {
  Repository(Ref ref) : super(‘’) {
    print(‘instantiate new Repository’);
    ref.watch(periodicProvider);
  }

  @override
  void dispose() {
    print(‘dispose the current Repository’);
    super.dispose();
  }
}

In model/service read and listen are safe but not watch, I believe.

So right now I don’t agree with the tone of the current document’s recommendation of watch. Rather it’d be better to encourage readers to learn each of functionalities of read/watch/listen and master when to use each of them.

rrousselGit commented 2 years ago

You're misunderstanding the reason why ref.watch is recommended.
Recommending ref.watch is about changing how you structure your code, such that ref.watch works as expected.

This is the equivalent of Implicit vs Explicit animations in Flutter. "Prefer ref.watch" is equivalent to "Prefer implicit animations", whereas ref.read/ref.listen/StateNotifier are the equivalent to explicit animations.

As such, if you were to use ref.watch, you'd probably not be using a stateful object to begin with.

reedom commented 2 years ago

I agree with that reactive/stateless software design is better than imperative/stateful ones. However, it's about application design; I'd say it's out of scope of the current library. Even though an intention of the library and document would be for the better design, it's not obvious from them. Rather, the library itself looks as more design agnostic, as walking through the API. So, I think that recommendation should be done somewhere else, e.g. blogs and/or appendix pages, but not in the API usage guide. And for the library users, to warn not to pass watch to other object would be one practical advices since it's simply harmful.

By the way, could you rewrite the todos example with read free? Then it'd enlighten me to employ more watches. :)

esDotDev commented 2 years ago

Agree. This guidance to put watch in everything it's basically based in a philosophy that virtually everything should be immutable / stateless. It doesn't work well when things do have state and you don't want them destroyed.

And I totally agree, it's punching above it's weight here. That's what I like about provider in comparison, less opinionated and works better with mutable data. It's a tool to share objects and bind to widget updates, I don't particularly care about the authors opinion on general application architecture (no offence).

This is why the docs are so hard to understand as well imo. If you come from the perspective of wanting to use mutable data it just acts like that concept doesn't even exist. You have to search hard to even find mention of it.

rrousselGit commented 2 years ago

I agree with that reactive/stateless software design is better than imperative/stateful ones. However, it's about application design; I'd say it's out of scope of the current library.

Why do you think that's out of the scope of the current library?

Riverpod has all the tools necessary for a stateless app. Both spec and the provider devtool use Riverpod to implement everything in a fully stateless way.

rrousselGit commented 2 years ago

This is why the docs are so hard to understand as well imo. If you come from the perspective of wanting to use mutable data it just acts like that concept doesn't even exist. You have to search hard to even find mention of it.

Why would Riverpod have to explain how to use ChangeNotifier when ChangeNotifier isn't made by Riverpod?

There are tons of articles on how to use ChangeNotifier on internet, and the mechanisms for interacting with providers is independent from whether you're using ChangeNotifier or StateNotifier. So I don't see what the value is in covering ChangeNotifier more in depth, when any other tutorial would work too.

That's in opposition with StateNotifier + immutability, which is both baked in Riverpod and with far fewer articles on the topic.

rrousselGit commented 2 years ago

In any case, the doc explain why we should use watch instead of read already, so I think this can be closed

esDotDev commented 2 years ago

For the same reason provider mentions it about 23 times in it's readme? Because it's a common pattern, and you want people to easily understand how to use your lib? Presumably?

I think it goes without saying not talking about how to use ChangeNotifier in the general sense. Rather, how to use ChangeNotifier/ValueNotifier specifically with this library.

All your docs and website are optimized for StateNotifier, which is just a ValueNotifier for immutable data, which is just assumed to be common knowledge, already throwing off new users. You wouldn't even know there is a ChangeNotifierProvider exists unless you specifically go hunting for it.

Anyways, it's just feedback, take it or leave it. If you feel it's invalid, ok, it's just how it has always felt to me. Just trying to give you some insight.

image

rrousselGit commented 2 years ago

For the same reason provider mentions it about 23 times in it's readme? Because it's a common pattern, and you want people to easily understand how to use your lib? Presumably?

Provider mentions it because Provider uses it. You don't see mentions about StateNotifierProvider in the Provider doc, right ?

Adding a page about it is planed. But there's a difference between that, and having multiple examples use ChangeNotifier like you seem to be requesting.

I think it goes without saying not talking about how to use ChangeNotifier in the general sense. Rather, how to use ChangeNotifier/ValueNotifier specifically with this library.

What exactly does that entails?

What is it that you think the website should mention about ChangeNotifier that isn't how to use ChangeNotifier in the general sense?

Because from my understanding, other than "You can use ChangeNotifierProvider to listen to a ChangeNotifier ", I don't see what else to say.

esDotDev commented 2 years ago

"Provider mentions it because Provider uses it." - I'm not sure what this means, provider could provide a StateNotifier, riverpod can provide a ChangeNotifier, this is non-sensical statement.

Feels like you're being intentionally obtuse here. Provider readme mentions in 23 times, riverpod mentions it zero. Does this not speak for itself? Does zero sound like the right number to you?

I'm not requesting anything, it's not a big deal to me whether your lib is easy to understand for users who like to work with mutable state or not. I'm quite happy with GetIt or Provider, I'm just trying to share some feedback on the tone of your docs.

What is it that you think the website should mention about ChangeNotifier that isn't how to use ChangeNotifier in the general sense?

For starters you could show a basic example of select on a change notifier. Mentioning that ChangeNotifierProvider even exists would be a another very obvious first step.

Your docs basically work backwards from:

It's nice if you're already into that world, it's really disorienting if you just came here looking for compile-safe DI + widget binding and are just bombarded with all these unrelated concepts.

rrousselGit commented 2 years ago

"Provider mentions it because Provider uses it." - I'm not sure what this means, provider could provide a StateNotifier, riverpod can provide a ChangeNotifier, this is non-sensical statement.

Provider has no StateNotifierProvider. A separate package is needed for that

That's different from Riverpod. Riverpod has no ChangeNotifier (you can use Riverpod without Flutter, at least I do) but does export StateNotifier+StateNotifierProvider

If Riverpod were to push mutable for real, it would not be using ChangeNotifier itself but rather export a custom class for it, such that it can be used outside of Flutter.

That's fine, but to begin with Riverpod is pushing immutability for various reasons. It's by no means limited to "The author prefers that"

Immutability is what makes things like ref.listen receive the previous value. And things like StateNotifier is what allows Riverpod from offering await ref.watch(stateNotifierProvider.future) if that StateNotifier uses AsyncValue. There's no equivalent with ChangeNotifier

Provider readme mentions in 23 times, riverpod mentions it zero.

Provider never mentions StateNotifier yet that's not a problem

Ultimately Riverpod will have some docs about ChangeNotifierProvider. But keep in mind that it's not made to be used extensively. ChangeNotifierProvider is here primarily as an easy migration tool for folks coming from Provider.

As such you'll find mentions of it in the upcoming "Riverpod for Provider users", because it makes senses there.

But in the other pages, StateNotifier makes more sense.

reedom commented 2 years ago

I agree with that reactive/stateless software design is better than imperative/stateful ones. However, it's about application design; I'd say it's out of scope of the current library. Why do you think that's out of the scope of the current library?

Riverpod has all the tools necessary for a stateless app. Both spec and the provider devtool use Riverpod to implement everything in a fully stateless way.

As dropping my opinion;

If there was a shoes(a tool) intentionally made for professional sprinters, I think its manual is not appropriate place to introduce any running technics, e.g. how long and which part of the sole should touch the ground, since that vary with individuals due to their traits. Plus, if there were people who have found any benefit in it for town walking but the maker was trying to send them to track fields, I'd have a strange feeling with the maker's behavior while people were satisfied with the product in their own ways. I want to see in its manual explain about shoes itself, like specs and how to maintain. It'd be okay to provide some hint to unleash its potential. But of running technics is a bit too much; better to be separated and such booklet would have high demand for juniors so that it might make a smash hit.

esDotDev commented 2 years ago

Fair enough! I don't want to try and tell you what to do with your own lib. Just more trying to illustrate the feelings you get when coming to riverpod initially if you do use a mutable flow. There's just no context and it's confusing. A few sentences go a long way. Something like: "While riverpod is generally optimized for immutable state with StateNotifer, users of mutable state can use ChangeNotfierProvider if desired." Would do a lot to just orient you in the right direction as a new user.

I do agree that there is no need to be so opinionated in the docs, instead just explain clearly what the API is, and why you would want to use one or the other api. It leads to a lot less dogma.

rrousselGit commented 2 years ago

Your opinion is not the majority though.

Docs are opinionated because that's exactly what the majority of people want

When most people complain about state management, they are saying "I want Flutter to be more opinionated on the topic".

When they ask for docs on how to use a state management package, they want very precise pointers on what should be preferred or avoided. They want to know where to place their classes, how to split their folders, and all other sorts of specific topics.

In fact there are quite a few poeple out there who argue that Riverpod is not opinionated enough.

reedom commented 2 years ago

Haha, now I just imagine for how many times you've been soaked by the rain of opinions of different colors. (Still I'm wondering how you've measured the majority rate, though.)

rrousselGit commented 2 years ago

Well the number of people who complained that Riverpod docs were too opinionated can be counted on one hand

Whereas people very regularly request for all forms of opinionated docs.
Some go as far as to say "I want to know how you, Remi, use Riverpod".

To begin with, as long as the tool is flexible, making the doc opinionated is probably the best case scenario.

Correct me if I'm wrong, but you two are likely more on the expert side. You should have the skills to extract the informations you care about from the docs, while ignoring the bits you personally disagree with. Whereas people who are looking for opinionated docs do so in part because they don't know how to decide and need official recommendations.

So you guys have the tools necessary to deviate from "my opinion" if you want. While folks who need recommendations get them

esDotDev commented 2 years ago

Ya, I understand there is a wide range of skill levels, and probably skews heavily to types that want their hands held (because flutter in general, is very hobbyist-heavy)

The problem with the docs being so opinionated, it is quickly becomes dogma, and then you get junior developers complaining to an experienced developer that they're doing something wrong, when they're really not, but to the junior, well it's right there in giant red bold font not to do this thing. Using phrases like "Use with caution" or "Make sure you know what you are doing", are better than "Never do X".

All good though, life goes on there are many options for the dev who wants to stick with mutable state for now. Thx for the back and forth!

Hopefully when meta-programming becomes a real thing we can all just move to immutable state flows, without the burden and complexity currently imposed by code-gen systems. Mutable state is the lesser of two evils for me personally.

lutes1 commented 1 year ago

Suppose I have such a scenario

class ServiceA extends ChangeNotifier {
  Object? someState;

  void updateState(Object? someNewState) {
    someState = someNewState;
    notifyListeners();
  }
}
final serviceAProvider = ChangeNotifierProvider((ref) => ServiceA());

class View extends ConsumerWidget {
  const View({super.key});

  @override
  Widget build(BuildContext context, WidgetRef ref) {
    final serviceA = ref.watch(serviceAProvider);
    ...
  }
}

class ViewModel extends ChangeNotifier {
  ViewModel(ServiceA service);
  ...
}

Suppose I want to watch the ServiceA in the View and have it notify the view about any change. Also I want to read the value of the service anytime I need it from the ViewModel without being notified for changes (for example in response to a button tap).

For the View I would obviously use ref.watch(serviceAProvider).

Naturally I would use ref.read(serviceAProvider) when creating the ViewModel since I don't need to observe any change and also since my ViewModel will be recreated in case ServiceA changes which is not really what I want:

final viewModelProvider = ChangeNotifierProvider.autoDispose((ref) {
  final serviceA = ref.read(serviceAProvider);
  return ViewModel(serviceA);
});

Passing the ref to ViewModel is a bad practice as it violates this, this, this and this.

Will I encounter any theoretical error or undefined behaviour with the framework if I would use .read(serviceAProvider) in the provider builder function?