rrousselGit / riverpod

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

[RFC] Unifying syntax for listening to providers (v2) #335

Closed rrousselGit closed 2 years ago

rrousselGit commented 3 years ago

This RFC is a follow-up to https://github.com/rrousselGit/river_pod/issues/246 with a slightly different proposal.

The problems are the same:

See https://github.com/rrousselGit/river_pod/issues/246 for a bit more explanation

Proposal

Instead of passing directly "watch" as parameter to widgets, Riverpod could do like with its providers and pass a "ref" object"

As such, instead of:

class StatelessExample extends ConsumerWidget {
  @override
  Widget build(BuildContext context, ScopedReader watch) {
    A value = watch(a);
  }
}

we'd have:

class StatelessExample extends ConsumerWidget {
  @override
  Widget build(BuildContext context, WidgetReference ref) {
    A value = ref.watch(a);
  }
}

Similarly, Consumer would become:

Consumer(
  builder: (context, ref, child) {
    A value = ref.watch(a);
  },
);

The behaviour would be strictly identical. But this then allows Riverpod to add extra methods on WidgetsReference, which could allow:

class StatelessExample extends ConsumerWidget {
  @override
  Widget build(BuildContext context, WidgetReference ref) {
    ref.listen<A>(a, (value) {
      print('provider a changed $value');
    });
  }
}

This would be equivalent to ProviderListener but without involving nesting.

Hooks consideration

While hooks_riverpod doesn't suffer from the problem listed at the start of the issue, the logic wants that hooks_riverpod should also use the same syntax too (both to reduce confusion and simplify maintenance).

As such, useProvider would be deprecated and a ConsumerHookWidget would be introduced. Which means that instead of:

class HooksExample extends HookWidget {
  @override
  Widget build(BuildContext context) {
    A value = useProvider(a);
  }
}

we'd have:

class HooksExample extends ConsumerHookWidget {
  @override
  Widget build(BuildContext context, WidgetReference ref) {
    A value = ref.watch(a);
  }
}

This would also clarify that the only purpose of hooks_riverpod is to use both hooks and Riverpod simultaneously.

context.read/context.refresh considerations

context.read(myProvider) and context.refresh(provider) would be deprecated.

Instead, ref should now be used. So the previous:

class StatelessExample extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return ElevatedButton(
      onPressed: () => context.read(counterProvider).state++;
    );
  }
}

would become:

class StatelessExample extends ConsumerWidget {
  @override
  Widget build(BuildContext context, WidgetReference ref) {
    return ElevatedButton(
      onPressed: () => ref.read(counterProvider).state++;
    );
  }
}

(and same thing with refresh)

This has two side-effects:

StatefulWidget consideration

An optional goal of this change is to support StatefulWidget.

This could be done by shipping a State mixin that adds a ref property, which would allow us to write:

class StatefulExample extends StatefulWidget {
   @override
  _StatefulExampleState createState() => _StatefulExampleState();
}

class _StatefulExampleState extends State<StatefulExample> with ConsumerStateMixin {
  @override
  Widget build(BuildContext context) {
    A value = ref.watch(a);
  }
}

Note that this is entirely optional, as Consumer can already be used.

ProviderReference vs WidgetReference

While the syntax for listening a provider in widgets and providers would now look similar with both being ref.watch(myProvider), it is important to note that ProviderReference and WidgetReference are distinct objects.

They are not interchangeable, and you could not assign WidgetReference to ProviderReference for example.

Their main difference is, ProviderReference does not allow interacting with ScopedProviders. On the other hand, WidgetReference do.

Similarly, it is entirely possible that in the future, some functionalities as added to one without being added to the other (such as https://github.com/rrousselGit/river_pod/pull/302 which allows ProviderReference to manipulate the state of its provider)

Conclusion

That's it, thanks for reading!

As opposed to https://github.com/rrousselGit/river_pod/issues/246, one major difference is that this proposal is "compile safe" without having to rely on a custom linter.
The downside is that the syntax for reading providers becomes a tiny bit more verbose.

What do you think of the proposed change?

Feel free to leave a comment. You can also use :+1: and :-1: to express your opinion. All feedbacks are welcomed!

Solido commented 3 years ago

Took me a while to decide but after some consideration using the new form for Hook makes code more readable. We can compose subsequent function using ref and not have to declare value as a property. This was not apparent the first time reading this RFC.

class HooksExample extends ConsumerHookWidget {
  @override
  Widget build(BuildContext context, WidgetReference ref) {
    A value = ref.watch(a);
  }
}
PiN73 commented 3 years ago

Doesn't ConsumerHookWidget break the prefer composition over inheritance rule?

If I need feature A (consume a provider) and feature B (use a hook like useTextEditingController), I will need to use ConsumerHookWidget class. But what if I need some feature C from a third package? Making ConsumerMyfeatureHookWidget doesn't seem the right way.

Hooks used to solve this problem by allowing to wire any feature through, well, hooks. But then, if I understand right, all features should be wired using hooks, without XxxYyyZzzHookWidget with @override Widget build(BuildContext, X, Y, Z) :)

For using without hooks there is already ConsumerWidget for base class and Consumer for nesting. Unifying their build/builder signatures to allow adding new features later seems OK.

PiN73 commented 3 years ago

Maybe it makes sense to make a "main" riverpod hook like useRiverpod which will return WidgetReference* and additional hooks like useProvider and useListen? These additional hooks will call useRiverpod internally. Adding new methods to WidgetReference and new hooks won't break anything

* maybe name it ProviderWidgetReference or something to emphasize it's not just plain flutter widget

derolf commented 3 years ago

Is there any particular reason, why it's not possible to watch a ProviderListenable? @rrousselGit

rrousselGit commented 3 years ago

@derolf It's just not implemented yet

rrousselGit commented 3 years ago

@PiN73 as explained earlier in this thread, the problem is about having to double the amount of work necessary for implementing a feature.

kmod-midori commented 3 years ago

I really do not like with the changes to hooks. It is tedious to migrate as I have many custom hooks with useProvider in them, now they have to receive an extra argument and I doubt that this process can be automated. Those hooks also call each other, adding noise into the code. One of the beautiful features of hooks is that they are easily composable and works as long as the order is correct, and now we have to pass that ref thing around, it outright looks bad.

rrousselGit commented 3 years ago

@chengyuhui I believe this should be automated too.

ResoDev commented 3 years ago

Will it be possible to call ref.read(provider) from initState() of a State mixed in with the ConsumerStateMixin?

creativecreatorormaybenot commented 3 years ago

That looks very nice. @rrousselGit will this be shipped exactly as currently described in the original issue?

rrousselGit commented 3 years ago

Yes.

Although there are some extra changes involved, mostly related to lower layers.

rrousselGit commented 3 years ago

As for what those lower layer changes involves if you're into that: It's a near-complete rewrite of how providers notify that their state changed

A side-effect is that container.listen now looks like a traditional addListener instead of the current weird API. This should make using Riverpod for command lines/servers easier.

There's also a chance that I'm going to remove the interfaces *ProviderBase in favor of making all providers implement Provider – which will involve a breaking change on context.refresh (but will be migrated with riverpod migrate too).

And you should now be able to make custom providers

Marco87Developer commented 3 years ago

I believe that, at this point, the introduction of any changes should not depend on the ease of rewriting the code. After all, the package is at version 0.x.y.

Personally, I really like the idea of ​​unifying the syntax, but I have some doubts about the use of the hooks. After the proposed change, what would be the advantage of using the hooks? Is syntax use... not characteristic of code that uses hooks? With the proposed change, I understand that the use of hooks would resort to a syntax that differs from that which generally characterizes hooks. Or did I get it wrong?

rrousselGit commented 3 years ago

The purpose of hooks_riverpod isn't to manipulate providers using useX, but rather to allow widgets to both use hooks and listen to providers at the same time.

For example, in the good old package:provider, you'd just mix context.watch with other hooks. But since Riverpod doesn't support context.watch, it needs an alternate solution.

If you don't need hooks, you don't need hooks_riverpod.

Marco87Developer commented 3 years ago

So the only hook that would disappear would be useProvider, while these hooks would remain. Did I get it right?

rrousselGit commented 3 years ago

Yes

Marco87Developer commented 3 years ago

OK, thank you very much! That said, I fully support the proposed changes!

TimWhiting commented 3 years ago

A side-effect is that context.listen now looks like a traditional addListener instead of the current weird API. This should make using Riverpod for command lines/servers easier.

I think you meant container.listen

scopendo commented 3 years ago

Hi @rrousselGit – thanks for the update. Your related tweet suggested to me that you'll be updating hooks_riverpod also, which I understood means a whole lot more work. I hate to think that hesitancy by others to adapt to one-off changes to a pre-1.0 package means that you'll have more work now and into the future. I actually call useProvider a fair bit but would happily adapt to better suit the long term maintainability of Riverpod.

As an aside, I wonder whether it might be better for Riverpod to switch from the term Provider to Pod, though I imagine that'd be a large undertaking:

Provider -> Pod StateProvider -> StatePod StateNotifierProvider -> StateNotifierPod

As always, appreciate what you do for the Flutter and Dart community.

rrousselGit commented 3 years ago

I decided against the renaming of providers, as explained here https://github.com/rrousselGit/river_pod/issues/197#issuecomment-803936752, because the votes showed that this was a controversial change.

I hate to think that hesitancy by others to adapt to one-off changes to a pre-1.0 package means that you'll have more work now and into the future

I am not sure what this means. Is there anything in particular you are unhappy with?

julienlebren commented 3 years ago

I have a question about the new syntax explained here:

class StatelessExample extends ConsumerWidget {
  @override
  Widget build(BuildContext context, WidgetReference ref) {
    return ElevatedButton(
      onPressed: () => ref.read(counterProvider).state++;
    );
  }
}

It was quite good to access providers' reading everywhere a BuildContext is available (or at least, where it can be passed as a parameter). I worry about the future of some parts of my code, because it would be a huge breaking change. For example, what will be the better rewriting for this kind of code:

  void initState() {
    super.initState();
    lastNameController.addListener(() {
      context.read(lastNameProvider).state = lastNameController.text;
    });
    firstNameController.addListener(() {
      context.read(firstNameProvider).state = firstNameController.text;
    });
  }

In the initState of a StatefulWidget, the BuildContext is available. If I understood your explanations, it won't be the case of WidgetReference. So I just can't figure it out how to do.

creativecreatorormaybenot commented 3 years ago

@julienlebren Your example is actually not suiting here because the change includes:

This could be done by shipping a State mixin that adds a ref property

Instead, it would certainly break any code where you have some function outside of State or build that only has access to the BuildContext.

rrousselGit commented 3 years ago

@creativecreatorormaybenot is correct. Your code will simply become:

class MyState extends State with ConsumerStateMixin {
  void initState() {
    super.initState();
    lastNameController.addListener(() {
      ref.read(lastNameProvider).state = lastNameController.text;
    });
    firstNameController.addListener(() {
      ref.read(firstNameProvider).state = firstNameController.text;
    });
  }

As for functions outside of widgets that take a BuildContext as a parameter, they will need to be refactored to take a WidgetReference as a parameter.

julienlebren commented 3 years ago

Thank you.

Refactoring does not worry me, I spend more time to refactor than writing new lines of code 🙄

scopendo commented 3 years ago

I decided against the renaming of providers, as explained here #197 (comment), because the votes showed that this was a controversial change.

That's fair enough – sorry, I missed that issue.

I hate to think that hesitancy by others to adapt to one-off changes to a pre-1.0 package means that you'll have more work now and into the future

I am not sure what this means. Is there anything in particular you are unhappy with?

Nope. To explain, I may have misunderstood but I had the impression from earlier in this RFC that maintaining hooks_riverpod adds quite a large burden on primarily @rrousselGit, that's all.

jeiea commented 3 years ago

I didn't read all comments, so please let me know if this question is duplicate.

I'm afraid of adopting WidgetReference because we can use useContext anywhere in build time but I'm not sure whether it is same to WidgetReference. If it's not possible I will pass WidgetReference in every method call.

Even if it's possible then I think I'll make similar one to useProvider.

Addition: I skimmed most of comments but couldn't resolve my concern.

rrousselGit commented 3 years ago

Once I am done with all the changes, I will re-evaluate the difficulty of implementing a useProvider. This the changes made for ref.listen, this may not be as hard as I thought. I'll keep you updated.

I would still prefer using the ref syntax though. That would significantly simplify the documentation.

venkatd commented 3 years ago

I really like the idea of unifying of how we read providers from providers and other widgets. It gives devs one less thing to learn and they understand things work the same in widgets and other providers.

One thing I've enjoyed is that a StatelessWidget can be converted to a HookWidget just by renaming StatelessWidget -> HookWidget.

The extra argument is one more thing to remember. What if context had an extension to get the ref like context.ref rather than adding an extra parameter? And if it's used in a StatelessWidget or somewhere it's not possible to use, we could provide a helpful error message.

ConsumerHookWidget also feels a bit longer to read vs. HookWidget. Why not make this a breaking change and update HookWidget directly?

rrousselGit commented 3 years ago

ConsumerHookWidget also feels a bit longer to read vs. HookWidget. Why not make this a breaking change and update HookWidget directly?

HookWidget isn't from Riverpod. Some people may use flutter_hooks without using riverpod. So this is not feasible

venkatd commented 3 years ago

HookWidget isn't from Riverpod. Some people may use flutter_hooks without using riverpod. So this is not feasible

My concern is one of usability and developer friction. Let me clarify.

Right now it's great that the only difference in StatelessWidget and HookWidget is the word Stateless and Hook. So "upgrading" from a StatelessWidget to a HookWidget is just a matter of renaming extends StatelessWidget to extends HookWidget. I find this very convenient and it is something I do all the time.

If we introduce a ConsumerHookWidget, I would have to deal with structural changes all the time:

What if, instead of introducing a ConsumerHookWidget with a 2nd argument, there was an extension for BuildContext so you could call final ref = context.ref and use it that way? (Or maybe a useRef()?) This way there aren't two types of HookWidgets. And the syntax of updating a StatelessWidget to a HookWidget just means replacing the word Stateless with Hook.

Or if not this, is there some other way way to allow the ref syntax in HookWidget so that StatelessWidget and HookWidget look the same as they do now and so we don't introduce another class to know about?

rrousselGit commented 3 years ago

I would rather have an IDE extension that offers utilities like "convert to HookConsumerWidget" & co, similar to the existing ones for Stateless/StatefulWidget.

venkatd commented 3 years ago

I think that would be a nice addition, but it still remains that there's another HookWidget to deal with and convert to-from and more transitions to deal with. And new users need know which one to use in each situation.

Every time I add/remove the use of providers or add/remove the use of hooks in a build method, I would be transitioning to a different widget.


  BEFORE                                                                       

┌─────────────────┐                                  ┌─────────────────┐       
│ StatelessWidget │◀────────────just rename─────────▶│   HookWidget    │       
└─────────────────┘                                  └─────────────────┘       

   AFTER                                                                       

┌─────────────────┐       ┌─────────────────┐       ┌─────────────────────────┐
│ StatelessWidget │◀─────▶│   HookWidget    │◀─────▶│   ConsumerHookWidget    │
└─────────────────┘       └─────────────────┘       └─────────────────────────┘
         ▲                                                       ▲             
         │                                                       │             
         └───────────────────────────────────────────────────────┘                        

I guess if something like a useRef was introduced to HookWidget so I could choose to stick with HookWidget, that could solve my use case. So if someone wants to use the ConsumerHookWidget, they can. Or if they want to stick to HookWidget in all situations, they also can.

creativecreatorormaybenot commented 3 years ago

@venkatd I think you are making this a bigger deal than it really is. The API benefits outweigh the preference in this case, I would say.

venkatd commented 3 years ago

Ok @rrousselGit @creativecreatorormaybenot I have changed my mind and I think the ConsumerHookWidget is actually a good idea.

I was making it out to be more complicated than it actually is.

If I want to use ref, I just prepend Consumer to HookWidget and add a WidgetReference argument. That's not too bad on second thought and it's more explicit than having some sort of useRef() thing.

As I understand, a ConsumerHookWidget would work exactly like a HookWidget except it would have access to ref? And then instead of useProvider I'd use ref as I use it in any provider declaration?

kmod-midori commented 3 years ago

I don't see adding an argument to all hooks that read provider data as an API improvement.

It is fine when converting existing widgets, as we only have two local changes, but to read a provider in an existing hook function, I would need to change all hooks that (either directly or indirectly) depend on this one. That's a lot of friction and I don't really like to depend on IDE plugins, which sometimes does not show up.

TimWhiting commented 3 years ago

@chengyuhui There is a command line tool which will migrate all hook functions that directly or indirectly use a provider. So there should be very little friction.

As for the API improvement, it allows for a lot more functionality (we don't have the limitations of hooks). We can listen to providers conditionally, we can choose to watch vs read vs listen to the provider, in general it is an improvement.

venkatd commented 3 years ago

@chengyuhui I thought the same initially but I think it's actually an improvement over before for most situations. If I want to use ref in a widget, I can prefix with Consumer add the ref argument. If I no longer need to use it, I drop the prefix and the argument.

Could you give an example of a hook you would need to change? I don't know if I'm following the example you're talking about. Do you mean hooks that currently call useProvider?

@TimWhiting @rrousselGit if a hook calls out to useProvider, how would that change with the new syntax?

TimWhiting commented 3 years ago

@venkatd

this gets migrated to this if you use the migration tool

dart pub global activate riverpod_cli 1.0.0-dev.1
riverpod migrate
jeiea commented 3 years ago

@TimWhiting Maybe what chengyuhui want to say is that cli tool's result, new recommended usage is not satisfactory.

As for the API improvement, it allows for a lot more functionality (we don't have the limitations of hooks). We can listen to providers conditionally, we can choose to watch vs read vs listen to the provider, in general it is an improvement.

I guess you wrote it is the limitation that we can't use hooks in conditional. It is a trait of hooks and tradeoff having nothing to do with riverpod. riverpod of course can have an api independent of hooks.

What I care is hook users still can use hooks and riverpod with minimal code. I was satisfactory to implicit parameter forwarding by use of hooks. I think previous api requires minimal code for using riverpod. But with rfc I don't know whether I must use HookConsumerWidget which is obviously seems verbose, and I can make and use useProvider equivalent one.

I agree watch, read, listen selectability is advantage. But I don't prefer uniformity sacrificing the minimal code said above.

It's a little off topic, I'm using hooks like the following due to this bug.

class UtilHooks {
  UtilHooks._();

  late BuildContext _context;

  static UtilHooks useInstance() {
    final utils = useMemoized(() => UtilHooks._());
    utils._useDependencies();
    return utils;
  }

  Future<void> alert() {
    return showDialog(context: _context, builder: (_) => AlertDialog(...));
  }

  void _useDependencies() {
    _context = useContext();
  }
}

I'm skeptical to think cli tool can cover all migration.

TimWhiting commented 3 years ago

@jeiea True, the migration tool only handles function hooks not class based hooks, I could support that, but it would take some more effort. Yes the tool doesn't cover all migration but it does cover 90%+ (just a guess), and if it doesn't please file issues so we can improve the migration tool.

I believe it should still be possible to create your own useProvider / useRef hook if you want to dig into how Riverpod works, but Remi shouldn't have to maintain that if he feels like his time is best spent elsewhere (in addition to separate documentation - which takes a lot of time).

I originally was of the same opinion that adding a parameter was verbose. However, I realized that one extra parameter really isn't a lot. I'd rather give Remi time to work on the riverpod devtool / linter in addition to potentially a riverpod solution for navigation etc and build the ecosystem more to make it easier to do common things. Most hooks users (including me) in this thread have gone through the same initial rejection of the idea followed by acceptance and sometimes even enthusiasm for it.

jeiea commented 3 years ago

I realized that one extra parameter really isn't a lot.

That sounds use of ConsumerHookWidget will be inevitable despite of custom useProvider, right?

Most hooks users (including me) in this thread have gone through the same initial rejection of the idea followed by acceptance and sometimes even enthusiasm for it.

Well I have not reached the time yet. I wrote my perspective for the reference. Anyway I have no choice after the library gets updated. I'm not rejecting whole change. I wish I can make use of previous code form if there's a room of compatibility.

venkatd commented 3 years ago

@jeiea I had the same opinion as you but I changed my mind.

This is because I realized using ConsumerHookWidget is pretty much the same amount of work as useProvider but we only have to learn one ref syntax.

Right now:

After the change:

The extra step takes a few seconds and then it's actually clearer than before because ref.watch is the syntax already being used when declaring providers that depend on other providers. Even if useProvider is available in the new version, I will probably want to switch to the new syntax because of clarity.

It may actually save time because we don't have to imagine in our head the difference between useProvider and ref.watch it's the same mental model.

assemmarwan commented 3 years ago

I can't seem to find ConsumerStateMixin, has it been removed? I want to use a StatefulWidget (or ConsumerStatefulWidget) yet be able to use useTextEditingController. How can I achieve that?

Currently when using ConsumerStatefulWidget, I get error saying that I can only call hooks from widgets that mix-in hooks.

Seferi commented 3 years ago

I can't seem to find ConsumerStateMixin, has it been removed? I want to use a StatefulWidget (or ConsumerStatefulWidget) yet be able to use useTextEditingController. How can I achieve that?

Currently when using ConsumerStatefulWidget, I get error saying that I can only call hooks from widgets that mix-in hooks.

Same here.. Is there any other way accesing provider in initState ?

rrousselGit commented 3 years ago

ConsumerStateMixin became ConsumerStatefulWidget + ConsumerState

It was not feasible to implement it with just a mixin

assemmarwan commented 3 years ago

@rrousselGit I see. I did that, but when using useTextEditingController I get the following:

======== Exception caught by widgets library =======================================================
The following assertion was thrown building UploadPage(dirty, state: _UploadPageState#10cb8):
Hooks can only be called from the build method of a widget that mix-in `Hooks`.

Hooks should only be called within the build method of a widget.
Calling them outside of build method leads to an unstable state and is therefore prohibited.
'package:flutter_hooks/src/framework.dart':
Failed assertion: line 134 pos 12: 'HookElement._currentHookElement != null'

Here's a snippet of the stateful widget:

class UploadPage extends ConsumerStatefulWidget {
  final File? image;

  UploadPage(this.image);

  @override
  _UploadPageState createState() => _UploadPageState();
}

class _UploadPageState extends ConsumerState<UploadPage> {
  bool uploading = false;

  @override
  Widget build(BuildContext context) {
    final nameController = useTextEditingController();

   .....
}
rrousselGit commented 3 years ago

If you want to use hooks, use hooks_riverpod with HookConsumerWidget

pikaju commented 3 years ago

While I like many of these changes, I do have a few points I would like to address:

Thank you for reading!

rrousselGit commented 3 years ago

have to modify pretty much all the widgets in my entire app and lock myself into using riverpod forever

That shouldn't lock you into using Riverpod forever. If it takes 2 seconds to switch from StatelessWidget to ConsumerWidget, it also takes 2 seconds to do it the other way around.

There are other cases where one might like to access providers in a widget tree, which are not just widgets.

There's no issue with this. Even if something isn't a widget, it will typically either have an ancestor or a descendent widget. So you should be able to obtain a ref anyway

I think a hook like useWidgetRef would work really well.

As mentioned previously, this requires too much work.

I personally don't see the issue with requiring hooks that want to read providers to receive a "WidgetRef" as parameter:

useSomething(WidgetRef ref) {
  useWhatever()
  ref.watch(provider);
}

The difference in verbosity is minimal.

debkanchan commented 3 years ago

This proposal makes a lot of sense for everything other than hooks consideration.

Adding ConsumerHookWidget and then using the migration tool is just creating a problem and making a solution for it. I've been using the dev version for some time now and having ref in build method feels intrusive. When I used just useProvider it was much uniform, cleaner and simple. And I think we should be striving for simplicity

As for the unifying syntax fact, I want to use hooks_riverpod BECAUSE I want to use hooks WITH riverpod. I WANT to use useProvider for the exact reason the syntax is different and simpler. I want to reap the benefits of hooks, hiding away the logic and stuff behind a simple hook. I get that your goal is using hooks_riverpod only to use hooks and riverpod together but that doesn't feel right. It makes hooks and especially using hooks and riverpod usage complicating and not intuitive. It's not a good experience for newcomers.

It should be simple (which is the case for non hooks consideration) and intuitive for the given context.