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

Add listen: false to Consumer #188

Closed
feinstein opened this issue Aug 5, 2019 · 58 comments
Closed

Add listen: false to Consumer #188

feinstein opened this issue Aug 5, 2019 · 58 comments
Labels
question Further information is requested

Comments

@feinstein
Copy link
Contributor

Sometimes I need just a Provider.of<T>(context, listen: false) but I can't use it since I don't have a BuilderContext with the type I want to access (they are all in the same Widget). Consumer using child is a good solution, but not a perfect one, since the builder method will execute for all Provider updates.

There are situations where we just want a value and no updates at all, but we don't have BuilderContext. We could just add a BuilderContext and use Provider.of, but it would be a lot more readable if we could just pass listen: false to a Consumer.

@rrousselGit
Copy link
Owner

I don't want to promote using listen: false.
What's your use-case?

@rrousselGit rrousselGit added the question Further information is requested label Aug 5, 2019
@feinstein
Copy link
Contributor Author

feinstein commented Aug 5, 2019

I create my model on a ProxyProvider on top of my builder method, as this model is only pertinent to that screen. Then I have a Scaffold, SafeArea and WillPopScope. WillPopScope only needs a simple bool from the model, so I wrapped it into a Consumer and passed the rest of the tree to the child (later I have other consumers down the child tree).

If this isn't clear, please let me know and I will post a snippet.

@rrousselGit
Copy link
Owner

I still do not understand why you want listen:false

@feinstein
Copy link
Contributor Author

feinstein commented Aug 5, 2019

So this is a simplification of my screen:

class _MyState extends State<MyScreen> {
  @override
  Widget build(BuildContext context) {
    return ChangeNotifierProxyProvider<FirebaseAuth, MyModel>(
      builder: (context, firebaseAuth, _) => MyModel(firebaseAuth),
      child: Scaffold(
        appBar: AppBar(
          title: Text('My App'),
        ),
        body: SafeArea(
          child: Consumer<MyModel>(
            builder: (_, model, child) {
              return WillPopScope(
                // Intercepting the Android Back button for Page Navigation
                  onWillPop: () async => model.isFirst,
                  child: child
              );
            },
            child: Column(
              children: <Widget>[
                Container(
                  // .....
                ),
                Consumer<MyModel>(
                  builder: (context, model, child) {
                    // .........
                  },
                ),
              ],
            ),
          ),
        ),
      ),
    );
  }
}

As you can see, WillPopScope will be rebuilt every time the model notifies its updates. But onWillPop is triggered asynchronously and it can get model.isFirst whenever it needs, it doesn't need an update on model.isFirst, so I don't need to rebuild WillPopScope at every new update from MyModel, which are a few, since this is a form and it will update at each keypress for form validation.

@rrousselGit
Copy link
Owner

Builder(
  builder: (context) {
    return WillPopScope(
      onWillPop: async () => Provider.of<Model>(context, listen: false).isFirst,
    );
  },
);

@feinstein
Copy link
Contributor Author

I thought about it, but I imagined it would be simpler to have listen: false on the Consumer, as it follows the same API of Provider.

@feinstein
Copy link
Contributor Author

Also, I remember someone telling me a while ago that Builder should be avoided when possible, just don't remember why, do you if this can have any side effects?

@rrousselGit
Copy link
Owner

It indeed would be simpler, but I do not want peoples to use listen:false.
Instead #169 is an alternative.

Also, I remember someone telling me a while ago that Builder should be avoided when possible, just don't remember why, do you if this can have any side effects?

There's nothing wrong with Builder.

@feinstein
Copy link
Contributor Author

I like this RFC, will track it, thanks.

@jtlapp
Copy link

jtlapp commented Aug 29, 2019

I do not want peoples to use listen:false.

I'm feeling lost again. I call Provider.of<Model>(context, listen: false) to give buttons access to models. The buttons don't need to be rebuilt. What should I do instead?

@feinstein
Copy link
Contributor Author

I think what he's trying to say is that he wants to avoid listen: false spreading all over the library, as this can make things more complicated, but you can use it with no problem.

@jtlapp
Copy link

jtlapp commented Aug 29, 2019

I think what he's trying to say is that he wants to avoid listen: false spreading all over the library, as this can make things more complicated, but you can use it with no problem.

Ah that makes more sense. I did read the context but assumed he was speaking more generally. Thanks!

@rrousselGit
Copy link
Owner

Yes, it's fine for event handlers.
But even then, it's quite unreliable. Peoples can forget to add the listen: false, or misuse it.

Selector is an example of a more rebust alternative.
Using ValueNotifier instead of ChangeNotifier with the custom provider I gave in the other issue is another alternative.

@feinstein
Copy link
Contributor Author

But how would he use Selector in this case, as he only wants a reference to his model inside a button, so there's no value to be tracked for the Selector.

@rrousselGit
Copy link
Owner

Similarly to how #188 (comment) used Consumer, but by having the Selector return model.isFirst

@jtlapp
Copy link

jtlapp commented Aug 29, 2019

Peoples can forget to add the listen: false, or misuse it.

I like this line of thinking. I'm sure to forget including this parameter myself and may only learn later should it end up being a performance problem. It would be nice to do something more explicit.

Selector is an example of a more rebust alternative.

I'll look into it, but on the surface it sounds like more work and more code than simply adding a listen: false argument.

@feinstein
Copy link
Contributor Author

Well, but then the Selector will rebuild the button each time isFirst changes, which is an undesired rebuild.

@rrousselGit
Copy link
Owner

The issue is more about ChangeNotifier than Provider.of

https://gist.github.com/rrousselGit/4910f3125e41600df3c2577e26967c91

This provider, using ValueNotifier instead of ChangeNotifier, do not suffer from this issue and do not need Selector/listen:false

@feinstein
Copy link
Contributor Author

IMHO seems more complicated than just using listen: false .

Maybe there could be a Provider named accordingly, so this behavior would be more explicit, like NonRebuildableProvider, so people won't have to remember to set listen: false, as this would be the normal mode of this Provider.

@jtlapp
Copy link

jtlapp commented Aug 29, 2019

NonRebuildableProvider

Or NonRebuildingProvider (ING) because provider doesn't itself rebuild. Or simply NoRebuildProvider.

@jtlapp
Copy link

jtlapp commented Aug 29, 2019

The sense of Provider<T>.of() may be completely backward. Had the default been listen: false, the dev would immediately notice the error when the widget didn't rebuild. Having default listen: true postpones discovery of a problem.

@feinstein
Copy link
Contributor Author

Yes, but having to type listen: true all the time when this is the most common use case is tedious.

But I agree that when I was reading about Provider I got a bit confused that it Provided values and also Rebuild the widget, by its name I only thought about it as a DI mechanism, maybe the names could indicate those behaviors as well and avoid all those little bugs.

@rrousselGit
Copy link
Owner

It listens by default, because not listening is more dangerous than listening.

It's better to have an extra rebuild than a missing rebuild. Especially considering that rebuilding is fast and no-op

@jtlapp
Copy link

jtlapp commented Aug 29, 2019

I'm now thinking that having a special NoBuildProvider is the wrong approach and the way things are is best. The provider designates a source of data and shouldn't capture the intentions of any of that data's consumers. Only the consumer knows whether it should be rebuilt.

@feinstein
Copy link
Contributor Author

I meant it as a NoRebuildProvider.of<int>(context) and not as the Provider.value we use upper on the tree.

@feinstein
Copy link
Contributor Author

NoRebuildConsumer would make more sense I guess. But still, we might start complicating the library as this might conflict with the Selector intentions... But in the end I kind like it, as Selector needs a value in order to make sense.

@jtlapp
Copy link

jtlapp commented Aug 29, 2019

I love the NoRebuildConsumer proposal! Yeah, NoRebuildProvider.of<T>() seems like a hack, because Provider.of<T>() is a request of the Provider class that we know we're using, but NoRebuildProvider isn't something we expect to know anything.

@feinstein
Copy link
Contributor Author

feinstein commented Aug 29, 2019

Actually, I think Provider.of is the issue. The library started with just the Provider class, and it was responsible for 2 thing: providing and consuming.

Then later came Consumer and now Selector. Maybe Consumer.of would have been the "correct" initial approach, at least for passing the meaning of the purpose of the class.

Maybe RebuildConsumer.of<> and NoRebuildConsumer.of<> should be used instead of Provider.of<>, which should be discouraged in the docs and deprecated later or even removed.... Or am I going too far?

@rrousselGit any thoughts on this? Maybe you already thought about it on the past?

@jtlapp
Copy link

jtlapp commented Aug 29, 2019

In both cases, NoRebuildConsumer is exactly what I don't want: ejecting reactivity inside the build method.

I'd assume NoRebuildConsumer<T>() would merely create an object that doesn't rebuild on changes to T but still otherwise rebuilds. If this is also undesirable, then I'm missing the downside.

There are other saner solutions. For example...

I think we have different definitions of "sane." Mind you, you've just proposed a complex per-model way to avoid having to add listen: false to a statement. If anything's going to replace (or supplement) listen: false it has to be less susceptible to error and of approximately the same complexity.

@feinstein
Copy link
Contributor Author

@rrousselGit I think what we are trying to propose is a way for the library to easilly deal with those cases, specially for beginners, and your solution is not an addition to the library, but to our client code.

@rrousselGit
Copy link
Owner

listen: false (or NoRebuildConsumer, as it's strictly the same) is dangerous.

With is, we can write codes that are statically correct but incorrect at runtime. This defeats the whole point of reactive/declarative programming.

For example, there's nothing preventing you to do:

return Text(Provider.of<String>(context, listen: false));

but it is obviously undesired.

If it was possible to infer it, I'd remove the listen entirely.

My dream is BuildOwner.debugBuilding was available in release mode, so that provider could know if we're inside a build method or in a click handler, to infer listen accordingly.

@rrousselGit
Copy link
Owner

If anything's going to replace (or supplement) listen: false it has to be less susceptible to error and of approximately the same complexity.

IMO that's a lot less complex than listen: false.

listen: false requires a cognitive effort to know when to use it or not, and can be misused. This solution, on the other hand, requires no logic at all. It's just a simple interface.

The boilerplate is boring. But that's only because of ChangeNotifier.

If I converted the same code to the ValueNotifier architecture I mentioned previously https://gist.github.com/rrousselGit/4910f3125e41600df3c2577e26967c91 (which uses the same principle), we'd have:

class Counter extends ValueNotifier<int> {
  Counter(): super(0);

  void increment() => value++;
}

With the following provider:

StoreProvider<Counter, int>(
  builder: (_) => Counter(),
  child: MyApp(),
)

And the consumers:

RaisedButton(
  onPressed: Provider.of<Counter>(context).increment,
  child: Text('+'),
);

and:

Text(Provider.of<int>(context).toString());

@jtlapp
Copy link

jtlapp commented Aug 31, 2019

Okay @rrousselGit, I think I'm understanding you now. You're agreeing that NoBuildConsumer (or possibly UnboundConsumer!) is logically equivalent to listen:false. Having one is no worse than having the other.

I like your ValueNotifier solution, but I'm not seeing the difference in cognitive effort. The consumer still has to decide whether to listen and choose a type accordingly.

The developer also has to do more work in the ValueNotifier solution.

@rrousselGit
Copy link
Owner

I like your ValueNotifier solution, but I'm not seeing the difference in cognitive effort. The consumer still has to decide whether to listen and choose a type accordingly

No its not needed anymore

@jtlapp
Copy link

jtlapp commented Aug 31, 2019

Oh wait! In your example, you can't message via Provider.of<int>(context), you can only message via Provider.of<Counter>(context), so the developer has no choice.

I think I'm following now! That is pretty elegant.

@rrousselGit
Copy link
Owner

Indeed. There's also no need for notifyListeners anymore. Just assigning .value is enough.

But this requires immutable data, which implicitly requires a code generator for the copyWith and stuff

@jtlapp
Copy link

jtlapp commented Aug 31, 2019

So let's say I want to expose multiple values from the same object. Maybe I have a record of lots of fields I'm displaying on the screen.

I could make a provider for each field, but that would get tedious. So instead I define a separate class for the record -- Record -- and then make a ValueNotifier<Record>.

The dev doesn't have to do that under the current paradigm, but it might be a better separation of concerns forcing the dev to do this.

I am not fond of code generators. When searching for a state management solution, I rejected those that generated code without further thought. In my experience they increase the maintenance burden -- keeping generated code properly in sync.

@rrousselGit
Copy link
Owner

I'm fine with code generators. They are built in Flutter now, and there's a package that makes a test that fails if the generated sources needs to be updated

@jtlapp
Copy link

jtlapp commented Aug 31, 2019

Code generators are fine when they occur under the hood and there is never a need for me to look at the code, much less know it was generated. That usually means the code is not mirroring my code elsewhere (unless of course it's transpiled).

@jtlapp
Copy link

jtlapp commented Aug 31, 2019

I think meantime, I'm going to make myself an UnboundConsumer for personal use. ;-)

@rrousselGit
Copy link
Owner

rrousselGit commented Aug 31, 2019

I hope that dart will increase its support of immutable class. That should remove the need for code generators.

As for your NoRebuildConsumer, it's indeed more logical on its own for now.

It's pretty straight forward anyway. It's just a Builder that calls Provider.of<T>(context, listen: false)

@feinstein
Copy link
Contributor Author

IMHO UnboundConsumer doesn't explain to a newbie what's going on.

@feinstein
Copy link
Contributor Author

And the ValueNotifier gets tricky when there's multiple values that a ViewModel has to pass.

@jtlapp
Copy link

jtlapp commented Aug 31, 2019

IMHO UnboundConsumer doesn't explain to a newbie what's going on.

Oh absolutely! But only with the present documentation. I'm proposing to revise the README.

I've proposed diagrams for explaining the architecture of this package. They introduce the binding language (that Remi suggested). Here's a summary of the architecture using this proposed language. Scroll up for various versions of the diagrams. (These diagrams are the ones I'm currently preferring.)

@feinstein
Copy link
Contributor Author

Diagrams are not my thing, I rather simple explanations and examples on what to do and what not to do and why.

@jtlapp
Copy link

jtlapp commented Aug 31, 2019

Diagrams are not my thing, I rather simple explanations and examples on what to do and what not to do and why.

Fair enough. That should be in the documentation too. I'm the kind of person who needs to see the abstraction, whether visually or formally in text.

@jtlapp
Copy link

jtlapp commented Sep 14, 2019

Okay, I'm posting this question here because it seems relevant to this discussion. I'm writing an article about the provider architecture and I want to talk about the following things:

  1. providers
  2. consumers that need to rebuild on state change
  3. widgets that need a reference to the state (e.g. a Listenable) but don't need to rebuild on state change

In the discussion above we came to call the last of these (3) "consumers" (e.g. NoRebuildConsumer), but do we really want to call them "consumers"? Are they actually "consuming" state? Should I find a better word to use in my article? (The horrible word "referencers" comes to mind.)

Right now my diagrams and articles call both (2) and (3) consumers.

@rrousselGit
Copy link
Owner

I still don't see the value behind such a thing.
In fact, based on my plans for a 4.0, there are situations where listen: false is not supported.

@rrousselGit
Copy link
Owner

FWIW I've opened a PR on Flutter that would depreciate listen in most situations: flutter/flutter#40491

This would allow provider to differentiate between:

Widget build(context) {
  Provider.of<T>(context);
}

and:

Widget build(context) {
  return RaisedButton(
    onPressed: () {
      Provider.of<T>(context);
    },
  );
}

such that in the former scenario it'll consider listen to be true, and in the latter listen will be false.

@jtlapp
Copy link

jtlapp commented Sep 14, 2019

I still don't see the value behind such a thing.
In fact, based on my plans for a 4.0, there are situations where listen: false is not supported.

My article describes the provider package as it exists right now. Right now, the dev has to explicitly pass listen: false.

I'm thinking that if your future implementation is going to fold non-rebuilding descendants into Consumer<T>, then I'm safe using one word -- consumers -- for both cases (2) and (3). So I think you've answered my question.

I'm looking forward to your future implementation -- one less thing for the dev to worry about.

@rrousselGit
Copy link
Owner

Well, Consumer will still rebuild in that situation.
You'd have to use Builder+Provider.of instead:

Builder(builder: (context) {
  return RaisedButton(
    onPressed: () {
      Provider.of<T>(context);
    },
  );
});

@jtlapp
Copy link

jtlapp commented Sep 14, 2019

Well, Consumer will still rebuild in that situation.
You'd have to use Builder+Provider.of instead:

Ah poo. Okay, it looks like I need to distinguish (2) and (3) after all. My diagrams are a bit simpler for calling both consumers, but that use is going to get more confusing moving forward.

As simple as this package is, it's been a challenge to accurately describe its architecture!

@rrousselGit
Copy link
Owner

rrousselGit commented Sep 14, 2019

Haha, that's because it's difficult to capture the simplicity of something into words/diagrams 😛

@jtlapp
Copy link

jtlapp commented Sep 14, 2019

I need a word for descendants of a provider that need access to the state, regardless of whether they need to rebuild on state changes. That word is going in most of my diagrams. Perhaps "dependent widget"? The article makes the listener distinction only about halfway through..

@rrousselGit
Copy link
Owner

The doc of provider uses descendants/dependents.

It's also the name of some core methods that makes InheritedWidgets works:

  • getDependencies
  • setDependencies
  • _dependents

@feinstein
Copy link
Contributor Author

This PR looks very interesting!

@Nialixus
Copy link

Nialixus commented Jan 7, 2022

I did encounter this problem where i need to use listen false inside consumer tree, and the answer it's quite simple.
You just need to "not do the inserting notifyListeners part" in ChangeNotifierProvider, for example :

class _Provider with ChangeNotifier{ 
   int key=0;

   void changeKey(int newKey){
   key=newKey;
   // No need NotifyListeners();
   }
}

and then use consumer as usual, it much more efficient rather than put things inside the builder of ChangeNotifierProvider (in my case). I hope this is the answer you're looking for. But if it's not than i'm sorry for misunderstanding the question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants