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

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

Closed
rrousselGit opened this issue Feb 19, 2021 · 115 comments
Closed

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

rrousselGit opened this issue Feb 19, 2021 · 115 comments
Labels
enhancement New feature or request

Comments

@rrousselGit
Copy link
Owner

rrousselGit commented Feb 19, 2021

This RFC is a follow-up to #246 with a slightly different proposal.

The problems are the same:

  • Riverpod needs a way to add new ways of "listening" to a provider (such as an equivalent to ProviderListener that doesn't involve nesting, but not exclusively)
  • Adding parameters to "build"/"builder" of ConsumerWidget/Consumer would be a large breaking change
  • StatefulWidgets may want to listen to providers without having to resort to Consumer
  • having a different syntax between reading a provider from another provider and reading a provider from a widget causes confusion

See #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:

  • StatelessWidget can no-longer be used to just "read" providers
  • there is no-longer a name collision between package:provider and Riverpod for context.read, which would simplify migration.

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 #302 which allows ProviderReference to manipulate the state of its provider)

Conclusion

That's it, thanks for reading!

As opposed to #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 👍 and 👎 to express your opinion.
All feedbacks are welcomed!

@dhannie-k
Copy link

Easier to read

@RobertBrunhage
Copy link
Contributor

RobertBrunhage commented Feb 19, 2021

I like most of it but I am not sure that I like the approach of a new ConsumerHookWidget.

When working with hooks we have a "pre-understanding" that we can use the useSomething statement to accomplish what we want, but the ConsumerHookWidget implements a complete new class just for this.

I don't really see how the benefits outweighing the negatives in this one. To me this would be a lot more verbose because we would have to refactor a HookWidget to a ConsumerHookWidget just to use the providers.

@davidmartos96
Copy link
Contributor

Following what @RobertBrunhage just said. If I'm not mistaken, the hook change would also mean that custom hooks that read from a provider now would need to receive a WidgetReference as a parameter.

@erf
Copy link

erf commented Feb 19, 2021

I like the new API !

I'm not familiar with riverpod_hooks, but i'm a bit confused that there are two separate packages / API's built on riverpod. Can someone explain why we need both riverpod_flutter and riverpod_hooks?

Also why have a ConsumerWidget / ConsumerHookWidget when you could just use a Consumer? I think removing the first two widgets would simplify the API and make it easier to get into.

@rrousselGit
Copy link
Owner Author

The ConsumerHookWidget is mainly for maintainability, to avoid having to duplicate work.

The issue with the current approach is, if I add a ref.listen, then I need to add a hook for it.

@gaetschwartz
Copy link

gaetschwartz commented Feb 19, 2021

I honestly like the proposal, only thing I don't really like is the necessity to use ConsumerWidget instead of StatelessWidget. For StatefulWidgets it's okay as adding a mixin is an easy migration but having to rewrite all StatelessWidgets as ConsumerWidgets is going to be tedious. Would it be possible to have something like a ConsumerStateMixin but for StatelessWidget ?

@rrousselGit
Copy link
Owner Author

No, a mixin on StatelessWidget is not feasible.

@rrousselGit
Copy link
Owner Author

@gaetschwartz do you have an example of what would he tedious to update?

Depending on the code, it may be doable to implement a migration tool.
No promise though

@rydmike
Copy link

rydmike commented Feb 19, 2021

This is quite a big breaking change imo. Then again Riverpod is still a 0.x.x version which basically includes that anything still goes from API dev point of view as well. Still for those already using Riverpod a lot of code will need to be changed, trivial refactors, but still quite a bit.

I actually don't like the new syntax that much, it is more verbose and not as pretty as before. Applies to the watch and especially hooks. Read is basically equivalent, so no real diff, but a refactor as well.

I like what this enables and appreciate that part, but it is not as pretty as before and anybody that has been using Riverpod will have quite a bit of work ahead of them to migrate to this new syntax.

On the plus side (in addition to the listeners) it does make things more consistent at the same time, which probably is less confusing for newcomers, so that is probably a good thing.

@gaetschwartz
Copy link

gaetschwartz commented Feb 19, 2021

Just saying that imagine having the most simple StatelessWidget:

class MyWidget extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return TextButton(
        onPressed: () {
          context.read(myProvider).doStuff();
        },
        child: const Text('Tap me!'));
  }
}

You have to:

  1. Replace StatelessWidget by ConsumerWidget - Feasible by just replacing all strings "StatelessWidget" by "ConsumerWidget".
  2. Change the signature of build - Can be a little longer but can also be done using replace and Regex.
  3. Replace all context.read() by ref.read(), the first - Can also be done by replacing "context.read" by "ref.read".

These changes can be trivial to advanced developers by using replace string/regex in VSCode etc but it is probably going to be tedious for beginners/newcomers. Providing instructions to semi-automatically migrate using regexp and search & replace on VSCode would be a good idea.

Tldr; if you are to make these changes it would be great to either give proper tools to migrate or make it easier to do so because as it is it's too much of a breaking change and would make it a long migration for middle/large sized projects. Where most users wouldn't actually see an improvement to their usage of riverpod so the cost/benefit wouldn't be optimal.

@RobertBrunhage
Copy link
Contributor

The ConsumerHookWidget is mainly for maintainability, to avoid having to duplicate work.

The issue with the current approach is, if I add a ref.listen, then I need to add a hook for it.

Have nothing against the maintainability part but I am afraid that in my point of view this will only make Hooks more complicated to get in to for newcomers.

Regarding what others have said about refactoring I agree that it will be a bit of a pain point but we also have to be aware that this is stated in the Readme file The API may change slightly when more features are added, and is something we should come to expect.

Personally I don't want to limit the refactoring part of making Riverpod easier to use before it hits 1.0.0 as that is when it will be harder to change these core implementations.

@rrousselGit
Copy link
Owner Author

rrousselGit commented Feb 19, 2021

About readability, it's worth noting that as Dart evolves, the syntax will likely get better naturally

For example, object destructuring could be a candidate:

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

@elianortega
Copy link

elianortega commented Feb 19, 2021

I agree with @RobertBrunhage on the last days I've been sharing content about Riverpod as a great tool, but people get afraid to fast. I think this change will only be a new tiny thing on the learning curve that riverpod brings with it.

@esenmx
Copy link

esenmx commented Feb 19, 2021

The only concern I have, since there is no real difference between context.read/refresh and ref.read/refresh(I think the only difference is ref lookup delay, which is not even a minor issue), context.read/refresh should be stayed as optional way. Depreciating it seems pointless to me.

@ToniTornado
Copy link

I like the proposal. Even for large projects it shouldn‘t be too much effort to migrate by updating Riverpod and just quickly fixing the Analyzer issues. I share the thoughts of @RobertBrunhage on ConsumerHookWidget but I can accept that for the sake of consistency and having a simpler API for beginners who probably start using Riverpod without hooks.

@rrousselGit
Copy link
Owner Author

rrousselGit commented Feb 19, 2021

On the plus side for hooks, ConsumerHookWidget allows conditions/loops:

Column(
  children: [
    if (something)
      Text(ref.watch(provider)),
    Text('Hello world'),
  ],
)  

This is not something doable with useProvider

@Marco87Developer
Copy link

I don’t understand something. If the syntax using hooks becomes the same as it would if we didn’t use hooks, what would be the added value of hooks?

@rrousselGit
Copy link
Owner Author

what would be the added value of hooks?

That you get to use hooks

So you can do:

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

@dancamdev
Copy link

dancamdev commented Feb 19, 2021

As a user of hooks_riverpod, I’m a bit concerned about the ConsumerHookWidget just like @RobertBrunhage said.

if the reason for it all is to have a WidgetReference wouldn’t it be possible to return it through an hook:

Widget build(BuildContext context) {
 final ref = useProviderRef()
 final provider = ref.watch(a);
}

and have a plain HookWidget

this would also allow for conditionals and loops

@TimWhiting
Copy link
Collaborator

TimWhiting commented Feb 19, 2021

I am in agreement with everyone. I share some concerns over deprecating useProvider. Mainly for reasons of creating / composing custom hooks that use providers. I think that @dancamdev's proposal of having a hook that returns a WidgetReference would be a good one if possible. However I also think that it would be nice to have the extra parameter in the build method to allow reading / watching in loops / conditionals, with fewer lines of code. I also like that it makes it easier to refactor between ConsumerWidget and ConsumerHookWidget, possibly an analyzer plugin might be able to add those refactorings too.

// In a custom hook
X useProviderXListenAndShowSnackbar() {
  final ref = useWidgetsReference();
  final context = useContext();
  final X = ref.listen(providerOfX, () {
    if (X.someCondition) {
      ScaffoldMessenger.of(context).showSnackBar(SnackBar());
    }
  });
  return X;
}

// Inside ConsumerHookWidget
Widget build(BuildContext context, WidgetReference ref) {
 final state = ref.watch(provider);
}

The main other complain I'm hearing is the tediousness of migration. I'm willing to help create a migration tool. I'll open another issue to talk about a migration tool.

@dancamdev
Copy link

@TimWhiting Just a clarification, I'm most probably wrong, as I often am, but by returning the reference through a hook, you should be free to call ref.watch(provider) in any conditional or loop. Again I'm no expert in how hooks work under the hood, so I may be wrong

@TimWhiting
Copy link
Collaborator

TimWhiting commented Feb 19, 2021

@dancamdev
I'm not saying that you can't do it exactly your suggested way. In fact you can and you should be able to use the WidgetReference in a loop just as you said.

I'm just saying that adding an extra parameter in the build method takes up fewer lines of code and makes the api more consistent allowing for easier refactorings between ConsumerHookWidget and ConsumerWidget for when you don't need hooks. Having the api more consistent also makes the documentation simpler.

But I agree that we need a hook for getting a WidgetReference for when you are not in the build method. Either way you should be able to use the WidgetReference in a loop. Unless I'm wrong, and it's not possible to get a hook to do this? Remi can correct me.

@symeonoff
Copy link

symeonoff commented Feb 19, 2021

final myProvider = StateProvider<bool>((ProviderContext context) {
  final another = context.read(anotherProvider);
  final oneMoreProvider = context.watch(oneMoreProvider);
});

class MyWidget extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    final another = context.read(anotherProvider);

    return Text(anotherProvider);
  }
}

class MyWidget extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Consumer(builder: (context, watch, _) {
    final oneMoreProvider = context.watch(oneMoreProvider);

    return Text(oneMoreProvider);
    })
    ;
  }
}

class MyWidget extends ConsumerWidget {
  @override
  Widget build(BuildContext context) {
    final oneMoreProvider = context.watch(oneMoreProvider);

    return Text(oneMoreProvider);
  }
}

class MyWidget extends ConsumerHookWidget {
  @override
  Widget build(BuildContext context) {
    final oneMoreProvider = context.watch(oneMoreProvider);

    return Text(oneMoreProvider);
  }
}

class MyWidget extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    final oneMoreProvider = context.watch(oneMoreProvider);

    return Text(oneMoreProvider);
  }

Everything is pretty much self explanatory. This way you can have context.container, context.refresh and etc.

@rrousselGit
Copy link
Owner Author

Everything is pretty much self explanatory. This way you can have context.container, context.refresh and etc.

context.watch is not feasible.

@assemmarwan
Copy link

assemmarwan commented Jul 9, 2021

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
Copy link

Seferi commented Jul 15, 2021

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
Copy link
Owner Author

ConsumerStateMixin became ConsumerStatefulWidget + ConsumerState

It was not feasible to implement it with just a mixin

@assemmarwan
Copy link

assemmarwan commented Jul 18, 2021

@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
Copy link
Owner Author

If you want to use hooks, use hooks_riverpod with HookConsumerWidget

@pikaju
Copy link

pikaju commented Aug 7, 2021

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

  • There are other cases where one might like to access providers in a widget tree, which are not just widgets. For instance, PageRoute<T> gives access to the Navigator's context, but is not itself a widget, so we cannot inherit from some ConsumerWidget type. Another example is TextSelectionControls with its buildHandle and buildToolbar methods. Providing even more special classes would not be sustainable in my opinion, I would greatly prefer some way of using the BuildContext instead.
  • As some people already pointed out, removing useProvider also removes the ability to compose custom hooks while using providers in the process. This very much so defeats the purpose of using hooks to begin with. For example, having a useTheme, useConfiguration, or useRouter hook alongside the inbuilt hooks could be cool, not to mention ones with much more complex behavior, but these all require access to providers. Note that React also supports accessing providers through hooks with the useContext hook. If your main concern is the syntax (i.e. using ref), I think a hook like useWidgetRef would work really well.
  • While I see the beauty in having a WidgetRef parameter in the build method, to me it is not a matter of personal preference or a "2 second change". I have to modify pretty much all the widgets in my entire app and lock myself into using riverpod forever, while losing a lot of compatibility. It feels very invasive, and makes me not want to use the package to begin with. flutter_hooks does this as well, which I also dislike, but in that case, it is literally impossible to do it any other way.

Thank you for reading!

@rrousselGit
Copy link
Owner Author

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
Copy link

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.

@jeiea
Copy link
Contributor

jeiea commented Sep 16, 2021

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.

I think it's hard to migrate hooks in automated way. Well it may be a special case of me, but I can't see contagious parameter insertion is trivial.

The difference in verbosity is minimal.

I don't think you didn't consider this, therefore I'm curious why there's disagreement in this issue.

useA() { useB(); }
useB() { useC(); }
useC() { useD(); }
useD() { useE(); }
useE() { useRepository(); }

The above will be

useA(ref) { useB(ref); }
useB(ref) { useC(ref); }
useC(ref) { useD(ref); }
useD(ref) { useE(ref); }
useE(ref) { useRepository(ref); }

It's similar to prop drilling. I think it is one of major reason of using DI library. If it's fine then why not use plain Map?

@scopendo
Copy link

scopendo commented Sep 16, 2021 via email

@satvikpendem
Copy link

Would functional_widget be updated to include ConsumerWidget/HookConsumerWidget? I previously used @hwidget with useProvider and it worked pretty well, cut down on a lot of boilerplate.

@venkatd
Copy link

venkatd commented Sep 17, 2021

I wonder how many feel that useProvider is simpler because they are used to it. A few months into the new syntax and I find the unification more intuitive.

I only have to understand ref.watch and ref.read rather than useProvider and ref.watch. It's one less concept I have to wrap my head around and it's the same amount of typing.

In many cases, we've got rid of hooks as they don't add much value vs. just calling ref.watch directly. For example, we had a useApi() which then became ref.watch(apiProvider). It's about the same amount of typing, but more explicit.

Aside from having to pass ref around in some cases, are there any other objections to the new syntax?

@rrousselGit
Copy link
Owner Author

rrousselGit commented Sep 17, 2021

Note that one thing I'm hopeful for is, once we have metaprogramming, we may be able to straight up remove ConsumerWidget/ConsumerHooKWidget and do:

final labelProvider = Provider((ref) => 'Hello world');

class Example extends StatelessWidget {
  @override
  Widget build(context) {
    final message = @watch(labelProvider);
    return Text(message);
  }
}

And it likely would work within hooks too.

Still, we'll see if the Dart team will really allow such macros.

@debkanchan
Copy link

I just read that and orgasmed. We want static metaprogramming so bad.

@creativecreatorormaybenot
Copy link
Contributor

I just read that and orgasmed. We want static metaprogramming so bad.

I do not like it that much - it surely makes the code less readable / more difficult to understand / to dive into as you cannot easily view the source.

It really is not any effort to write out ConsumerWidget.

@rrousselGit
Copy link
Owner Author

rrousselGit commented Sep 18, 2021

To be clear, using metaprogramming here isn't about reducing a bit the numbers of characters we have to type.

The primary use-cases are:

  • performance, as through code-generation we could change the implementation to be a whole lot faster
  • improved scoped provider support.

In particular, a common problem with the v1, where we can now scope all providers, is that folks tend to override a provider but forget to override its dependencies. Such that we have:

final provider = Provider((ref) => 'Hello')
final another = Provider((ref) => ref.watch(provider))

ProviderScope(
  overrides: [
    provider.overrideWithValue('Bonjour'),
  ],
  child: Consumer(
    builder: (context, ref, _) => Text(ref.watch(another)) // will print Hello, not Bonjour
  )
)

The fix is to do:

ProviderScope(
  overrides: [
    provider.overrideWithValue('Bonjour'),
    another, // tell Riverpod to scope the 'another' provider too
  ],

But it seem confusing.

So one solution I have in mind, providers could list their dependencies like so:

final provider = Provider((ref) => 'Hello')
final another = Provider((ref) => ref.watch(provider), dependencies: {provider});

such that when we override provider, this will override another too. But having to specify the dependencies is tedious.

Hence the idea of using metaprogramming here. Where folks would write:

final provider = Provider((ref) => 'Hello')
final another = Provider((ref) => @watch(provider));

and that dependencies: {provider} would be generated by the macro for you. That should fully fix the problem.

@pikaju
Copy link

pikaju commented Sep 22, 2021

Please take people's criticism seriously, Remi. I agree with all the criticism mentioned so far.

I would also like to put more emphasis on the argument of @scopendo, in that it would be great to use the context object for retrieval, the same way it works with the provider package. What exactly is the motivation for not using something along the lines of context.watch(counterProvider)? The syntax would be the same, except for the word context, but one could argue that this would justify renaming the Provider construction's parameter ref to context. This would also solve the problem with flutter_hooks, as it would be possible to write useContext().watch(counterProvider). And it makes sense in cases where only a context but no ref is available. Also you don't need to change the widget base class (big win). In general it seems extremely convenient, and removes the need for metaprogramming in the build method.

I'm aware you mentioned that this is not feasible, perhaps I just misunderstood the architecture, could you elaborate further? I'd imagine if provider can do it, so can riverpod? This being a widely used package, I would personally place the importance of a clean way to use the package over how clean the implementation is.

I clearly see the advantages of riverpod over provider, but with the way things are moving now I would begrudgingly switch back.

@rrousselGit
Copy link
Owner Author

I'm aware you mentioned that this is not feasible, perhaps I just misunderstood the architecture, could you elaborate further? I'd imagine if provider can do it, so can riverpod?

One of the very reason Riverpod is a separate package from Provider is because context.watch isn't reasonably doable.

context.watch works by using InheritedWidgets to rebuild widgets. ref.watch isn't relying on InheritedWidgets, but rather a combination of "addListener" + "setState".
This is a necessary requirement for various reasons, be it fixing bugs with InheritedWidgets or adding support for new features or performance.

Using context would require:

  • removing autoDispose – because it isn't possible for InheritedWidgets to know when a listener is removed)
  • removing family – because widgets are incapable of conditionally listening to an InheritedWidget
  • limiting the numbers of providers of an application, because notifying listeners would be O(N^2) where N is all the listeners in the application (including the consumers that don't listen to the updated provider)
    • or alternatively we'd have to list all the providers of our application in the main.dart like with package:provider

In comparison, using ConsumerWidget instead of StatelessWidget is a benign change.

@TimWhiting
Copy link
Collaborator

@pikaju
Seeing as it is infeasible to go back to the 'context' way of doing things, is there anything that we can do to make the transition easier for you? For example we have the riverpod_cli package that provides a migration tool to migrate automatically from the pre-1.0.0 riverpod to the latest dev version.

Additionally I'm interested in helping Remi with an analyzer plugin for riverpod that will likely include refactorings like the 'Wrap with widget' and 'Extract Widget' refactorings that are available from flutter, for example we could do:

  • Convert to ConsumerWidget
  • Convert to ConsumerHookWidget
  • Convert back to Stateful / Stateless widget
  • Convert from ConsumerWidget to ConsumerStatefulWidget
  • Add ref parameter to a hook & everywhere that hook is called from (this might be a bit harder, but is being done in the migration tool currently for existing usages)
  • etc

Also I've discussed with Remi how to provide a migration tool from the old provider package to the riverpod package for legacy codebases.

So if there is any change that you think would improve the experience of riverpod over provider without going back to the limitations of provider, I'm sure the suggestions would be welcome.

@venkatd
Copy link

venkatd commented Sep 23, 2021

@pikaju criticism is taken into consideration, but there are many different opinions on how riverpod should be designed, and the and some some difficult tradeoffs have to be considered.

I personally love that riverpod is not coupled to BuildContext or Flutter at all. This makes testing pure Dart code much easier and would allow reuse of riverpod in other contexts such as AngularDart.

Changing the base class is very fast. You have to to add/remove Consumer or HookConsumer and add/remove the WidgetRef parameter. It takes less than 5 seconds.

The main criticism that might be valid is that, if a hook depends on ref, that parameter would now need to be drilled through all the hooks that need it. But again, it was a tradeoff that had to be made. I personally prefer passing this along since it's a more explicit dependency.

In our codebase, hooks tend to be reserved for more general purpose functions for state management.

Perhaps you could share some code samples of where you find your developer experience is worse with the new riverpod syntax? Without something specific, it will be hard to provide suggestions or find ways to improve riverpod.

@pikaju
Copy link

pikaju commented Sep 23, 2021

Thank you for the clarification @rrousselGit, I think I understand the problem in much more detail now. I also took a look at the flutter_riverpod, hooks_riverpod, flutter_hooks, and even the Flutter framework source code to see how things currently work under the hood, and spent a lot of time trying to think of possible improvements. I admit it is very tough. Prepare for a lot of rambling.

My main goal was to apply the "composition over inheritance" rule to both flutter_riverpod as well as flutter_hooks. Turning ConsumerState into a ConsumerStateMixin and simply not passing the ref into the build function. I honestly prefer that over a new base class with new parameter sets, but it doesn't change much, and doesn't solve the problem of having an XxxYyyZzzWidget in hooks_riverpod, as @PiN73 called it, or in this case, an XxxYyyZzzWidgetMixin. So I thought about changing flutter_hooks to have a HookWidgetMixin instead of a HookWidget (bear with me). Unfortunately, that does not work because both packages replace the default Element types with their own subtypes to implement their special behavior, making them somewhat mutually exclusive by nature, which, like you, I also didn't find a way around. I thought about creating a new package containing a compositional Element type that packages like flutter_hooks and flutter_riverpod can depend on in order to harmonize with one another, but this is assuming there is a need for more widgets of this style.

Eventually I thought it would be better to just create a subtype of InheritedWidget that is capable of detecting when a listener is removed, so that we can roll with context.watch, but when I saw how the Flutter framework implements dependency removal, I flipped my table. Perhaps some Flutter pull requests would help to go the context.watch route, what do you think?

TLDR: You guys did the best you could, I surrender. All hail our lord and savior, metaprogramming. If at all possible, please try adding useWidgetRef.

@TimWhiting I appreciate the effort, but I'm personally not a big fan of automatic code conversion tools.
@venkatd Keep in mind that flutter_riverpod is already detached from riverpod. I'm only looking for ways to improve the Flutter/hooks bindings.

@rrousselGit
Copy link
Owner Author

rrousselGit commented Sep 23, 2021

Believe me, I spent a lot of time trying to get BuildContext to work.
I'm open to suggestions, but I haven't found any approach that wouldn't have fundamental flaws.

And that includes making PRs in Flutter to change how InheritedWidgets works. I made a few experiments before Riverpod was a thing, with no success.

Ultimately while it's a bit sad to have to rely on ConsumerWidget, I don't see that as a real issue.
Flutter already comes with a lot of Widget type; far more than 3 that is. Adding one or two more seem innocent to me.


I wouldn't place too much hope in metaprogramming though.

While I would love to be able to do final value = @watch(provider) and remove ConsumerWidget/HookWidget, the Dart team doesn't seem too interested in the use-case.
They haven't straight-up rejected it, but I highly doubt that the initial release will allow us to do that.

@rrousselGit
Copy link
Owner Author

Time to finally close this issue. Thanks for everyone's participation!

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

No branches or pull requests