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

Document good practices around "listen" flag #244

Closed
jtlapp opened this issue Oct 14, 2019 · 42 comments
Closed

Document good practices around "listen" flag #244

jtlapp opened this issue Oct 14, 2019 · 42 comments
Labels
documentation An opportunity for improving the documentation

Comments

@jtlapp
Copy link

jtlapp commented Oct 14, 2019

In issue #188 we discussed adding a class analogous to Consumer but which doesn't rebuild on value changes. It seemed to me that you, Remi, were against the idea because you had better plans. Those plans appeared to be contingent on Flutter making debugBuilding available in release mode. The Flutter team declined, so I'm here formally proposing a convenience class.

I'm using this class in my own code because sometimes it makes for cleaner code; that's all Consumer is doing, anyway. I'm calling it Dependent:

import 'package:flutter/material.dart';
import 'package:provider/provider.dart';

class Dependent<T> extends StatelessWidget {
  Dependent({Key key, @required this.builder, this.child});

  final Widget Function(BuildContext cx, T value, Widget child) builder;
  final Widget child;

  @override
  Widget build(BuildContext cx) {
    final value = Provider.of<T>(cx, listen: false);
    return builder(cx, value, child);
  }
}

class Dependent2<T1, T2> extends StatelessWidget {
  Dependent2({Key key, @required this.builder, this.child});

  final Widget Function(BuildContext cx, T1 value1, T2 value2, Widget child)
      builder;
  final Widget child;

  @override
  Widget build(BuildContext cx) {
    final value1 = Provider.of<T1>(cx, listen: false);
    final value2 = Provider.of<T2>(cx, listen: false);
    return builder(cx, value1, value2, child);
  }
}

We also discussed naming this class NoRebuildConsumer and UnboundConsumer. Another possibility is NonlisteningConsumer. I'm kind of partial to Dependent because the dependent can still call Provider.of<T>() for some other type and therefore still rebuild.

@jtlapp
Copy link
Author

jtlapp commented Oct 14, 2019

I'm proposing it here in a new issue because that discussion was actually a sidetrack of issue #188, which was closed for other reasons.

@rrousselGit
Copy link
Owner

Hello!

To be honest I still don't understand the use case

@jtlapp
Copy link
Author

jtlapp commented Oct 14, 2019

To be honest I still don't understand the use case

The use case is pretty much anywhere you could use listen: false. Here's an example lifted from my code:

            child: Dependent<User>(builder: (cx, user, child) {
              return RaisedButton(
                child: Text(textFor('button_add')),
                onPressed: () => submit(cx, user),
              );
            }),

It's analogous to Consumer:

    child: Consumer<User>(builder: (cx, user, child) {
      return Padding(
        padding: const EdgeInsets.all(10.0, 0.0, 10.0 20.0),
        child: Text("User: ${user.name}",
      );
    }),

Both of these are merely convenience classes, making the code shorter and easier to understand. Here's the longer equivalent for the above code:

            child: Builder(builder: (context) {
              final user = Provider.of<User>(context, listen: false);
              return RaisedButton(
                child: Text(textFor('button_add')),
                onPressed: () => submit(context, user),
              );
            }),

    child: Builder(builder: (context) {
      final user = Provider.of<User>(context);
      return Padding(
        padding: const EdgeInsets.all(10.0, 0.0, 10.0 20.0),
        child: Text("User: ${user.name}",
      );
    }),

There are also times when you can't use Consumer, such as when you need to get the value from didChangeDependencies(). I haven't yet encountered a case where I couldn't use Dependent if I needed it, but this might happen in a generic function that doesn't know the widget's listening status.

@jtlapp
Copy link
Author

jtlapp commented Oct 14, 2019

Ah poo. That wasn't the best example. Replace User with something dynamic like some object that the user is creating.

@rrousselGit
Copy link
Owner

Builder(builder: (cx, user, child) {
  final user = Provider.of<User>(cx, listen: false);
  return RaisedButton(
    child: Text(textFor('button_add')),
    onPressed: () => submit(cx, user),
  );
}),

I don't like that code. I think it's a bad practice to call Provider.of<Foo>(context, listen: false) inside build.

IMO the call should be moved inside the callback:

Builder(builder: (cx, user, child) {
  return RaisedButton(
    child: Text(textFor('button_add')),
    onPressed: () {
      submit(cx, Provider.of<User>(context, listen: false));
    }
  );
}),

That's a lot safer because we can't mistakenly use the obtained value to build a Text for instance.

@rrousselGit
Copy link
Owner

rrousselGit commented Oct 14, 2019

Similarly, calling Provider.of inside the callback instead of build allows some sanity checks.

Even if debugBuilding is not available in release mode, it's still available in debug mode, and we can use it to make sure everything works as expected. See #236

@jtlapp
Copy link
Author

jtlapp commented Oct 14, 2019

Even if debugBuilding is not available in release mode, it's still available in debug mode, and we can use it to make sure everything works as expected. See #236

Okay, so you're looking at changing provider so that this usage of Dependent would throw an exception when debugging because Provider can't tell if the listen value is right. That would certainly rule out this proposal.

because we can't mistakenly use the obtained value to build a Text for instance

I didn't understand that. What if I want the following?

Builder(builder: (context) {
  final model = Provider.of<Model>(context);
  return RaisedButton(
    child: Text(model.count == 0 ? "Add First" : "Add Another"),
    onPressed: () => submit(cx, model),
  );
}),

@rrousselGit
Copy link
Owner

I didn't understand that. What if I want the following?

That's completely different since the value is used by builder and listen is true.

My previous comment is to avoid:

Widget build(context) {
  final foo = Provider.of<Foo>(context, listen: false);
  return Text('$foo');
}

That should never happen.

@jtlapp
Copy link
Author

jtlapp commented Oct 14, 2019

That should never happen.

I was about to provide an example where the text is pulled from a value and the widget does not listen to changes in value. But then I realized that in this case, it doesn't matter whether listen is false or not and could easily have been true.

So the code you provided isn't necessarily an error, but it's also not necessary to code it this way.

It's safe to listen: true for a value that doesn't change but unsafe to listen: false for a value that does change. You want to do what you can to help the dev avoid the latter situation.

@jtlapp
Copy link
Author

jtlapp commented Oct 14, 2019

My biggest beef with Provider is the Provider.of method. I have to pass a parameter that says what I mean by this method. I'd rather have to type a method name that says what I mean. This would greatly help devs from accidentally doing the wrong thing.

We'd have to be more conscious of the decision doing something like this, and the code would be clearer too:

value = Provider.updatingValue<T>(context);
value = Provider.nonUpdatingValue<T>(context);

@rrousselGit
Copy link
Owner

Even if the part of the value used never changes, listen: false should not be specified if it's to build the widget tree.

As the app scale, the "it never changes" may not be true anymore. This will cause a very hard to catch bug (the impacted widgets may not even be in the git diff).

There are better alternatives, like Selector or the upcoming support "aspects" #232.

@jtlapp
Copy link
Author

jtlapp commented Oct 14, 2019

Even if the part of the value used never changes, listen: false should not be specified if it's to build the widget tree.

I'm slowly starting to understand. I'm seeing some best practices:

  • Always use Consumer (not Provider.of<T>(context, listen: false)) when building the tree. Values that are unchanging now may be changing in the future, and you don't want to have to revise all your descendent widgets.
  • Only ever call Provider.of<T>(context, listen: false) in response to user actions, never during the build process. That way you won't accidentally fail to rebuild widgets as required.

@jtlapp
Copy link
Author

jtlapp commented Oct 14, 2019

I wonder if this is an argument for making the following changes:

  • Consumer is the only way to get a value that listens to changes.
  • Provider.of only returns a non-listening value (but this would break existing code).

Alternatively, add a more helpfully named method for returning non-listening values:

value = Provider.nonUpdatingValue<T>(context)

And maybe also deprecate listen: false.

@jtlapp
Copy link
Author

jtlapp commented Oct 14, 2019

Or maybe to emphasize that it can't be called while building:

value = Provider.nonbuildingValue<T>(context);

@rrousselGit
Copy link
Owner

Exactly

This is enhanced by the fact that the "build" method is extremely fast. It doesn't really matter if a widget rebuilds too often and won't cause any issue.

The sanity impact of using listen: false to not rebuild one widget is not worth it IMO.

Provider.of only returns a non-listening value (but this would break existing code).

Provider.of is legitimately used in didChangeDependencies. That wouldn't work anymore.

Alternatively, add a more helpfully named method for returning non-listening values:

listen: false is fine I think. Having too many methods is harmful. And there are issues like: #235, which will make things worse.

If folks want a custom method instead of Provider.of(ctx, listen: false), they can use extension members.

We should be able to do:

extension on Provider {
  static nonBuildingValue<T>(BuildContext context) {}
}

final value = Provider.nonBuildingValue<T>(context);

@jtlapp
Copy link
Author

jtlapp commented Oct 14, 2019

The sanity impact of using listen: false to not rebuild one widget is not worth it IMO.

I'd think this depends on the application. But we can use that argument to drop the listen parameter, because it never belongs in a rebuild.

Provider.of is legitimately used in didChangeDependencies. That wouldn't work anymore.

Oh, right. I'm even doing that where I need a changing value at initialization.

listen: false is fine I think. Having too many methods is harmful. And there are issues like: #235, which will make things worse.

I'm finding the listen parameter harmful. I've been misusing it, as you've seen.

Now I'm even more in favor of dropping of and having these two clearer methods instead:

value = Provider.buildingValue<T>(context);
value = Provider.nonBuildingValue<T>(context);

That's only one more method that what we have now. It will break everything, but developers will be left a lot saner, and Provider's proper usage easier to understand.

If folks want a custom method instead of Provider.of(ctx, listen: false), they can use extension members.

I did think of that, coming from a Kotlin background. I already had the dart extension spec pulled up in another tab. It would be a pain in the butt to always have to include my own dart file to get them, though.

@rrousselGit
Copy link
Owner

rrousselGit commented Oct 14, 2019

Now I'm even more in favor of dropping of and having these two clearer methods instead:

I don't think there's any difference between both besides causing problems for #235

Instead of passing listen: false, folks will just nonBuildingValue. That's the same thing.

I did think of that, coming from a Kotlin background. I already had the dart extension spec pulled up in another tab. It would be a pain in the butt to always have to include my own dart file to get them, though.

You can combine it with the export directive:

// my_provider.dart
export 'package:provider/provider.dart';

extension Foo on Provider {
  static whatever() {}
}

Then just import my_provider.dart instead of package:provider/provider.

@rrousselGit
Copy link
Owner

rrousselGit commented Oct 14, 2019

I'd think this depends on the application. But we can use that argument to drop the listen parameter, because it never belongs in a rebuild.

I was thinking of making a PR on flutter to makeStatefulElement._debugLifecycleState & _StateLifecycle public.

This way provider would be able in debug mode to differentiate between initState,build... Such that listen: false would be allowed only in initState/didUpdateWidget (and event handlers of course)

@jtlapp
Copy link
Author

jtlapp commented Oct 14, 2019

// my_provider.dart
export 'package:provider/provider.dart';

extension Foo on Provider {
  static whatever() {}
}

Then just import my_provider.dart instead of package:provider/provider.

Hey that's awesome! I really am liking Dart more and more. I thought the nodejs module management system (and CommonJS) were fantastic improvements on the chaos of Java's package system, now I'm thinking that Dart has even one-upped that.

@jtlapp
Copy link
Author

jtlapp commented Oct 14, 2019

Okay, I see that I need to abandon the Dependent proposal, but are there any other takeaways from this discussion requiring action?

Maybe the docs need to say more about the usage of Consumer and listen? I guess that should wait until Provider actually fails with listen: true when not building.

@rrousselGit rrousselGit added documentation An opportunity for improving the documentation and removed discussion labels Oct 14, 2019
@rrousselGit
Copy link
Owner

I'll keep this open as a reminder to document the good practices of listen (those listed here #244 (comment))

@yaymalaga
Copy link

Do you also recommend these good practices of listen when using Provider + MobX?

I would love to have more details on good practices in general, like for example:

  • Is it fine to send the BuildContext to the store? For example to navigate to another screen or to show a toast, so this way the full logic is outside the widget.
  • Is it fine to define a ScrollController in the store instead of on a stateful widget? For example to use a bottomBar icon to reset a view's scroll position.

@rrousselGit
Copy link
Owner

  • Is it fine to send the BuildContext to the store? For example to navigate to another screen or to show a toast, so this way the full logic is outside the widget.
  • Is it fine to define a ScrollController in the store instead of on a stateful widget? For example to use a bottomBar icon to reset a view's scroll position.

No and no.

Your store should by principle have no dependency on Flutter.

@yaymalaga
Copy link

So how would you deal with these things? By using global keys? I just see "simple" provider examples tutorials, but not more complex real ones, what is a shame.

This kind of problems confuse me when thinking of the use of stores/models. As I see them, it is an opportunity to also split out the UI and the logic, so the state remains outside too. But still we need to use an stateful widget to create a controller for example.

@rrousselGit
Copy link
Owner

rrousselGit commented Oct 22, 2019

Usually a callback does the trick.

Instead of:

class MyModel {
  MyModel(this.context);
  final BuildContext context;

  void myMethod() {
    final anotherModel = Provider.of<AnotherModel>(context);
    ...
  }
}

we can have:

class MyModel {
  MyModel(this.getModel);
  final T Function<T>() getModel;

  void myMethod() {
    final anotherModel = getModel<AnotherModel>();
    ...
  }
}

where is it created through:

Provider(
  create: (context) => MyModel(<T>() => Provider.of<T>(context)),
)

We can also do:

class MyModel {
  MyModel(this.anotherModel);
  final AnotherModel anotherModel;

  void myMethod() {
    ...
  }
}

Provider(
  create: (context) => MyModel(Provider.of(context, listen: false)),
)

or:

class MyModel {
  void myMethod(AnotherModel anotherModel) {
    ...
  }
}

Provider(
  create: (context) => MyModel(),
)


Provider.of<MyModel>(context).myMethod(Provider.of(context));

@rrousselGit rrousselGit changed the title Non-listening convenience class analogous to Consumer Document good practices around "listen" flag Nov 30, 2019
@frank06
Copy link

frank06 commented Dec 10, 2019

@rrousselGit

The code above doesn't seem useful for real-world models.

What if I have other arguments in my constructor?

class MyModel {
  MyModel(this.anotherModel, this.id, this.title);
  final int id;
  final String title;
  final AnotherModel anotherModel;

  void myMethod() {
    ...
  }
}

Sure I can easily inject AnotherModel, but id and title?

Provider(
  create: (context) => MyModel(Provider.of<AnotherModel>(context),  /* params here somehow? */),
)

What would be a good approach using Provider?

Or Provider is not good for this and I'm better off using a classic service locator? Would mixing a locator be a good idea?

@rrousselGit
Copy link
Owner

I don't get your question.

@frank06
Copy link

frank06 commented Dec 10, 2019

Which part you didn't get exactly?

Consider my immutable Post model that has a dependency on your MyModel:

class Post {
  final int id;
  final String title;
  final MyModel model;

  Post({
    @required this.id,
    @required this.title,
    @required this.model
  });
}

I wire up the MyModel dependency:

Provider(
  create: (context) => Post(model: Provider.of<MyModel>(context), id: ???, title: ???),
)

But... how do I get hold of id and title in order to initialize this Post?

Does my question make sense?

@rrousselGit
Copy link
Owner

Can't you just pass whatever you want to the constructor?

I don't see how that relates to provider

@frank06
Copy link

frank06 commented Dec 12, 2019

I'm trying to inject a dependency in a model. Since Provider is touted as a DI solution for Flutter and the examples above show how to inject dependencies in "models" I thought for a moment it was possible.

There's two problems:

  • I would need this to be a factory, i.e. inject dependency and return a new initialized instance (with constructor arguments, as it's immutable) on request – but I figure Provider can't do this. Sorry for my confusion!
  • Even if it could, the UI layer shouldn't know anything about models dependencies.

So I'm now using a service locator to achieve this:

class Post {
  final int id;
  final String title;
  final MyModel model = ServiceLocator.of<MyModel>();

  Post({
    @required this.id,
    @required this.title
  });
}

I simply call Post(id: 1, title: 'foo') and it's done, no need for Provider. I think I heard it wasn't a good idea to use Provider with a service locator, but I don't recall why. Plus there doesn't seem to be an alternative.

@rrousselGit
Copy link
Owner

provider is a service locator. Combining it with a service locator makes no sense.

I still don't understand what you're trying to do.
Don't explain what you think is the solution, explain the problem. Otherwise, you may fall for the x-y problem.

@frank06
Copy link

frank06 commented Dec 12, 2019

I want this to work:

// ...
return GestureDetector(
  onDoubleTap: () {
    Post(id: 2, title: 'bar').model.doSomething();
  },
  child: Text(Post(id: 1, title: 'foo'))
);

model has to be injected into all Post instances (without the client having to do that manually for each instance like Post(id: 1, title: 'foo', model: Provider.of<MyModel>())). This is possible with something like get_it.

@rrousselGit
Copy link
Owner

That specific syntax is purposefully not possible because providers are scoped.
get_it uses global variables, which provider explicitly refuses to use.

If you're looking to do that, consider simply storing your model in a global variable instead of using provider/get_it.
That's the same thing in the end.

@frank06
Copy link

frank06 commented Dec 12, 2019

Thanks! It's clear now.

consider simply storing your model in a global variable instead of using provider/get_it.

Yes. Using something like Repository.of<MyModel>(); inside my classes right now.

Therefore it makes sense to use Provider in the UI layer (I really like ChangeNotifierProvider), and registering/resolving services via a "global map<type, service>" in non-UI layers, ala Kiwi or Get_it.

@rrousselGit
Copy link
Owner

I disagree with using both provider and another service locator making sense
It's more that you're fighting against the library.

Ultimately it's your decision, but these are voluntary limitations and using globals or another service locator on the top of it is anti-pattern.

provider could also store all objects in globals so that we'd be able to do what you want.
But it doesn't, for the sake of code-health.

@frank06
Copy link

frank06 commented Dec 12, 2019

Would you mind explaining why it's "unhealthy" exactly? And why combining the approaches is an anti-pattern.

I come from Ember.js which is a very well designed framework and injecting globally registered services is common practice.

@rrousselGit
Copy link
Owner

provider goes in the continuation of the widget system.
And widgets come with a set of voluntary restrictions to make the app more scalable.
provider respect these rules and requires users of provider to respect them too.

As such, providers are:

  • scoped
  • using a unidirectional data-flow.

This grant in turn:

  • composability
  • overridability
  • testability

But combining provider with a global-based approach goes against both scope and uni-directional data-flow.

For example, you could use have more than a single instance of your model at a time:

Provider<Model>(
  create: (_) => Model(),
  child: MaterialApp(
    routes: {
      '/tutorial':  (_) => Provider<Model>(
        create: (_) => MockedModel(someValue: 42),
          child: Home(),
        ),
      ),
      '/': (_) => Home(),
    },
  ),
)

In this app, both the / and /tutorial screens use the same Home widget, which depends on Model.
But /tutorial uses an overridden instance of Model with a bunch of mocked data.

But that is not something you can represent using globals.

Similarly, unidirectional data-flow is a very good tool to improve the scalability of your app.
But globals, again, go against it.

@frank06
Copy link

frank06 commented Dec 12, 2019

I agree those are practices to strive for, and this is why I use Provider in the UI layer. (Even if service locators are definitely overridable/testable)

But dependency injection is also required outside the UI layer (in the model or service layers, that know nothing about widgets). How do you use Provider in that case?

Actually – Do you have a multi-layered app example using Provider? Cause so far I have only found very short snippets, maybe this is why I'm having trouble understanding.

@frank06
Copy link

frank06 commented Dec 13, 2019

Okay so what I understand is that all dependency injection should be configured in the UI layer with Provider, for "global" services something like the following:

  Widget build(context) {
    return MultiProvider(
      providers: [
        Provider.value(value: Dio()),
        ProxyProvider<Dio, Api>(
          update: (_, dio, __) => Api(dio)
        ),
        ProxyProvider<Api, PostsRepository>(
          update: (_, api, __) => PostsRepository(api)
        ),
        ProxyProvider<Api, CommentsRepository>(
          update: (_, api, __) => CommentsRepository(api)
        ),
       // long long list ...
      ],
      child: MaterialApp(

or would you use other tools for constructor injection? So that network-layer concerns are not exposed in the UI layer

Another minor consequence of only using Provider is that these services can't be accessed before runApp, if needed

@jlnr
Copy link

jlnr commented Mar 1, 2020

I've accidentally built a complete app thinking that Provider.of would never add a dependency, and that Consumer was the way to follow value updates, as it was suggested in #244 (comment). I still think it would be more intuitive to use widgets (Consumer) for building, and functions (.of) for/in actions. But I guess that ship has sailed :)

I'll probably introduce a custom extension Provider.currentValue<T> to fix all my event handlers after upgrading to Provider 4.x, but I really wish that of could be split into two methods in Provider itself (as proposed in #244 (comment)). Provider has become such a staple in the Flutter community that most developers can read code that uses it, adding app-specific extension methods takes away from that.

@rrousselGit
Copy link
Owner

but I really wish that of could be split into two methods in Provider itself

That's what the 4.1.0 is supposed to do, along simplifying Selector

@rrousselGit
Copy link
Owner

Closing this, since these good practices are now enforced with the 4.1.0 extensions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation An opportunity for improving the documentation
Projects
None yet
Development

No branches or pull requests

5 participants