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] Simplifying providers that depends on each others #46

Closed
rrousselGit opened this issue May 1, 2019 · 18 comments
Closed

[RFC] Simplifying providers that depends on each others #46

rrousselGit opened this issue May 1, 2019 · 18 comments

Comments

@rrousselGit
Copy link
Owner

From experience looking at larger Flutter applications, it is relatively common to have some dependency between providers.

For example, we may have a Provider<Configuration> and a Provider<AuthentificationService> where the latter depends on the former.

But that's currently not something we can represent purely using provider package. Our only solution right now is to somehow eject provider and handle everything ourselves like so:

Configuration configuration;

Widget build(BuildContext context) {
  return MultiProvider(
    providers: [
      Provider.value(value: configuration),
      Provider(builder: (_) => AuthentificationService(configuration)),
    ],
    child: MaterialApp(...),
  );
}

That's not good.
It reintroduces the ability to make a circular object graph & forget to update/dispose of an object.
We're loosing all the fancy securities that using widgets for DI brings.


The discussion on #44 made me realize that an advanced Consumer may be able to solve this issue.

Naively we could think of doing:

MultiProvider(
  providers: [
    Provider<Foo>(builder: (_) => Foo()),
    Consumer<Foo>(builder: (context, foo, child) {
      final bar = Bar(foo);
      return Provider<Bar>.value(value: bar, child: child);
    }),
  ].
  child: MaterialApp(...),
);

... The problem is that we're doing final bar = Bar(foo); inside the build method of Consumer.
That's not good either – we may have memory leaks.
But that's something we can fix using a more advanced Consumer, which I'll call ProxyProvider for now.

The idea behind ProxyProvider is that it would work similarly to the default constructor of Provider, but also like Consumer as it would read values from other providers and pass them to the builder.

The same example using ProxyProvider would be:

MultiProvider(
  providers: [
    Provider<Foo>(builder: (_) => Foo()),
    ProxyProvider<Foo, Bar>(builder: (context, Foo foo, Bar previousBar) {
      return Bar(foo);
    }),
  ].
  child: MaterialApp(...),
);

In that example, the builder of ProxyProvider would be called on the first build; or whenever Foo changes.

@rrousselGit
Copy link
Owner Author

rrousselGit commented May 1, 2019

One of the possible implementations could be:

class ProxyProvider<T, R> extends StatefulWidget
    implements SingleChildCloneableWidget {
  const ProxyProvider({
    Key key,
    this.builder,
    this.updateShouldNotify,
    this.child,
  }) : super(key: key);

  final R Function(BuildContext, T, R) builder;
  final UpdateShouldNotify<R> updateShouldNotify;
  final Widget child;

  @override
  _ProxyProviderState<T, R> createState() => _ProxyProviderState();

  @override
  ProxyProvider<T, R> cloneWithChild(Widget child) {
    return ProxyProvider(
      key: key,
      builder: builder,
      updateShouldNotify: updateShouldNotify,
      child: child,
    );
  }
}

class _ProxyProviderState<T, R> extends State<ProxyProvider<T, R>> {
  R value;

  @override
  void didChangeDependencies() {
    super.didChangeDependencies();
    value = widget.builder(context, Provider.of<T>(context), value);
  }

  @override
  Widget build(BuildContext context) {
    return Provider<R>.value(
      value: value,
      child: widget.child,
      updateShouldNotify: widget.updateShouldNotify,
    );
  }
}

We may want to give the ability to use a provider other than Provider<R>.value using a named constructor.

@filiph
Copy link
Collaborator

filiph commented May 2, 2019

Ah, is this what you meant over at #44? I love this idea as it's a common use case and a clean API. From first glance, it doesn't look like there are perf implications.

I wonder what @jiaeric thinks.

@rrousselGit
Copy link
Owner Author

Ah, is this what you meant over at #44?

It's related, but not exactly. My remark on #44 got me thinking about potential use-cases/flaws, and it here we are. 😄

I think that's an interesting idea. But I want to make sure that we don't solve an issue by introducing a bigger problem down the road.

The current example is relatively easy. I'll think of more complex scenarios with a combination of:

  • im/mutable objects
  • updateShouldNotify
  • combining it with ChangeNotifierProvider/similar
  • dispatching setState or similar within the value builder

There are some questions that need to be answered first I think. Like "when do we dispose of the created value?" because some objects may be recreated while others may be mutated.

@filiph
Copy link
Collaborator

filiph commented May 2, 2019

Yeah, this could get pretty gnarly before things are satisfactorily answered. If you agree, I would put this outside 2.0.0 scope.

@rrousselGit
Copy link
Owner Author

I would put this outside 2.0.0 scope.

No problem! That's not a priority anyway, it's just fuel for discussions.

@filiph
Copy link
Collaborator

filiph commented May 10, 2019

Coming back to this: what's the implication when a provider depends on multiple other providers? I don't have a use case at hand, but I can imagine it will crop up. Can we compose ProxyProviders inside each other?

@rrousselGit
Copy link
Owner Author

rrousselGit commented May 10, 2019

Currently, providers can't depend on other providers. Not in a sane way at least.

Our "only" options are:

  • Wrap the provider that depends on another provider into a Consumer. That works fine as long as the object is immutable and don't need to be disposed of – which is rare in the ChangeNotifier architecture.
  • use Provider.of<Foo>(context, listen: false). Works fine if there's no update to do. It works with BLoC, but the ChangeNotifier architecture suffers again.
  • Make a custom StatefulWidget and handle everything ourselves. Tedious and error-prone.

Can we compose ProxyProvider

Definitely!

MultiProvider(
  providers: [
    Provider<Foo>(...),
    ProxyProvider<Foo, Bar>(
      builder: (context, foo, previous) => Bar(foo),
    ),
    ProxyProvider<Bar, Baz>(
      builder: (context, bar, previous) => Baz(bar),
    ),
  ]
);

I assume we'll want variations of the ProxyProvider that consumes 1-6 values from other providers.


Similarly, I think it'd be interesting to have an optional providerBuilder argument.

ProxyProvider<Foo, Bar>(
  builder: (context, foo, previous) => Bar(foo),
  providerBuilder: (context, bar, child) => ChangeNotifier.value(value: bar, child: child),
  child: <whatever>
)

and the default behavior of providerBuilder would be

(context, value, child) => Provider.value(value: value, child: child),

@MarcinusX
Copy link

I've tried to use ProxyProvider, but without providerBuilder you have to ditch ChangeNotifierProvider which I find easiest to use.

Is the following setup matching what you meant when suggested wrapping in Consumer?
Assuming that DataService is just to inject methods and it doesn't have any actual state in it.

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return Provider<DataService>.value(
      value: DataService(NetworkService(), LocalService()),
      child: Consumer<DataService>(
        builder: (context, dataService, child) {
          return MultiProvider(
            providers: [
              ChangeNotifierProvider.value(
                notifier: CountriesViewModel(dataService),
              ),
            ],
            child: child,
          );
        },
        child: ....
    );
  }
}

@rrousselGit
Copy link
Owner Author

rrousselGit commented May 16, 2019

What do you mean by:

but without providerBuilder you have to ditch ChangeNotifierProvider which I find easiest to use.

?

There's providerBuilder in the current implementation available there: #59

And there's an example using it here

Assuming we go forward with ProxyProvider, I'll add a few shorthands to make using ProxyProvider with existing providers easier. Like ProxyChangeNotifierProvider and such.

@rrousselGit
Copy link
Owner Author

Is the following setup matching what you meant when suggested wrapping in Consumer?

Yes but that still fails.
Assuming that DataService is immutable, CountriesViewModel still isn't.

Using your code snippet, you'll easily lose your state when rebuilding.

@MarcinusX
Copy link

Oh, I don't know how I missed it... Sorry for that...

Yeah, you are right about losing state... So do I understand correctly that withProxyProvider it is handled because of using previous in the builder like in the example you just mentioned?

And thank you for those explanations! You're the best!

@rrousselGit
Copy link
Owner Author

it is handled because of using previous in the builder like in the example you just mentioned?

Yes. That previous exists for mutable objects. Immutable ones usually won't care about this argument.

filiph added a commit to filiph/samples that referenced this issue May 16, 2019
After this change, we’re managing Counter’s lifecycle with ChangeNotifierProvider.

This removes the periodic Timer. Not only does that simplify the example and makes it closer to the original, it also prevents a leaking timer (though, in this case, it’s not an issue, since the timer is needed for the whole duration of the app). I experimented with a more robust approach (having or injecting a Timer/StreamController into the Counter, and disposing of it there) but that seemed overly complex for such a simple example. This whole problem will get significantly easier with rrousselGit/provider#46, at which point I could reintroduce this. I will also think about a more complex Provider example, something like the venerable `bloc_complex`, with infinite-scrolling pagination and all that.
@bizz84
Copy link

bizz84 commented May 17, 2019

I've been playing with providers with inter-dependencies.

One example I currently have looks like this:

  static Widget create(BuildContext context) {
    final auth = Provider.of<AuthBase>(context);
    return Provider<ValueNotifier<bool>>(
      builder: (context) => ValueNotifier<bool>(false),
      child: Consumer<ValueNotifier<bool>>(
        builder: (context, valueNotifier, _) => Provider<SignInManager>(
              builder: (context) =>
                  SignInManager(auth: auth, isLoading: valueNotifier),
              child: Consumer<SignInManager>(
                builder: (context, manager, _) => ValueListenableBuilder<bool>(
                      valueListenable: valueNotifier,
                      builder: (context, isLoading, _) => SignInPage(
                            manager: manager,
                            isLoading: isLoading,
                          ),
                    ),
              ),
            ),
      ),
    );
  }

This could be reduced to:

  static Widget create(BuildContext context) {
    final auth = Provider.of<AuthBase>(context);
    return Provider<ValueNotifier<bool>>(
      builder: (context) => ValueNotifier<bool>(false),
      child: ProxyProvider<ValueNotifier<bool>, SignInManager>(
          builder: (context, valueNotifier, _) =>
              SignInManager(auth: auth, isLoading: valueNotifier),
          child: Consumer<SignInManager>(
            builder: (context, manager, _) => ValueListenableBuilder<bool>(
              valueListenable: valueNotifier,
              builder: (context, isLoading, _) => SignInPage(
                manager: manager,
                isLoading: isLoading,
              ),
            ),
        ),
      ),
    );
  }

Beyond that, it would be nice to use MultiProvider as in #46 (comment).

After some experimentation, I ended up with something like this:

  static Widget create(BuildContext context) {
    final auth = Provider.of<AuthBase>(context);
    return MultiProvider(
      providers: [
        Provider<ValueNotifier<bool>>(
            builder: (_) => ValueNotifier<bool>(false)),
        ProxyProvider<ValueNotifier<bool>, SignInManager>(
          builder: (_, valueNotifier, _) =>
              SignInManager(auth: auth, isLoading: valueNotifier),
        ),
      ],
      child: Consumer<ValueNotifier<bool>>(
        builder: (_, valueNotifier, _) => Consumer<SignInManager>(
              builder: (_, manager, _) => ValueListenableBuilder<bool>(
                    valueListenable: valueNotifier,
                    builder: (_, isLoading, _) => SignInPage(
                          manager: manager,
                          isLoading: isLoading,
                        ),
                  ),
            ),
      ),
    );
  }

Does this look reasonable?

Also, it would be nice to simplify the 3 nested consumers I currently have.
Maybe something like a ProxyConsumer?

In any case, I like the direction Provider is taking, thanks a lot!

@rrousselGit
Copy link
Owner Author

Also, it would be nice to simplify the 3 nested consumers I currently have.

There's a variation of Consumer which consumes more than one value: Consumer2, ..., Consumer6

ProxyProvider would follow the same treatment. So in your example we may use ProxyProvider2 instead, which would listen to both AuthBase and ValueNotifier<bool> at the same time.
Otherwise SignInManager isn't rebuilt when AuthBase changes.
NIT if AuthBase never changes, but it's cool to keep the providers reactive.

Is SignInManager listening to the ValueNotifier<bool>? If so, you're likely missing on a dispose. Which means this usage of ProxyProvider is likely invalid.

All of these combined, you may want that instead:

ProxyProvider2<ValueNoftifier<bool>, AuthBase, SignInManager>(
  builder: (_, isLoading, auth, previous) {
    final manager = previous ?? SignInManger();
    manager.auth = auth;
    manager.isLoading = isLoading;
    return manager;
  },
  dispose: (_, manager) => manager.dispose(),
)

Alternatively can change Provider<ValueNotifier<bool>> into ListenableProvider<ValueNotifer<bool>>.

Which means that SignInManager do not need to listen to the ValueNotifier, and may, therefore, become immutable.

Which also means that we can replace our previous example with:

ProxyProvider2<ValueNoftifier<bool>, AuthBase, SignInManager>(
  builder: (_, isLoading, auth, _) => SignInManger(auth, isLoading.value),
)

@MarcinusX
Copy link

Do you have any example on how ProxyProvider2,3,4... should look?

@rrousselGit
Copy link
Owner Author

ProxyProvider2 would be syntax sugar for:

ProxyProvider<Foo, Bar>(
  builder: (context, foo, previous) {
    final baz = Provider.of<Baz>(context);
    return Bar(foo, baz);
  }
)

@bizz84
Copy link

bizz84 commented May 18, 2019

@rrousselGit thanks for all the info!

In my case SignInManager doesn't listen to ValueNotifier, only sets the value.

However you're right about disposing the ValueNotifier. I should be doing this in the dispose closure of Provider<ValueNotifier<bool>>.

So this is what I came up with:

    return MultiProvider(
      providers: [
        Provider<ValueNotifier<bool>>(
          builder: (_) => ValueNotifier<bool>(false),
          dispose: (_, valueNotifier) => valueNotifier.dispose(),
        ),
        ProxyProvider<ValueNotifier<bool>, SignInManager>(
          builder: (_, valueNotifier, __) => SignInManager(auth: auth, isLoading: valueNotifier),
        ),
      ],
      child: Consumer2<ValueNotifier<bool>, SignInManager>(
        builder: (_, valueNotifier, manager, __) => ValueListenableBuilder<bool>(
              valueListenable: valueNotifier,
              builder: (_, isLoading, __) => SignInPage._(
                    manager: manager,
                    isLoading: isLoading,
                  ),
            ),
      ),
    );

I really like that I can use Consumer2 with types that have inter-dependencies. As in, SignInManager takes ValueNotifier<bool> as an input, but this is taken care of by the ProxyProvider. So I can just list the types I need with ConsumerX. Correct?

filiph added a commit to filiph/samples that referenced this issue May 20, 2019
After this change, we’re managing Counter’s lifecycle with ChangeNotifierProvider.

This removes the periodic Timer. Not only does that simplify the example and makes it closer to the original, it also prevents a leaking timer (though, in this case, it’s not an issue, since the timer is needed for the whole duration of the app). I experimented with a more robust approach (having or injecting a Timer/StreamController into the Counter, and disposing of it there) but that seemed overly complex for such a simple example. This whole problem will get significantly easier with rrousselGit/provider#46, at which point I could reintroduce this. I will also think about a more complex Provider example, something like the venerable `bloc_complex`, with infinite-scrolling pagination and all that.
filiph added a commit to flutter/samples that referenced this issue May 20, 2019
After this change, we’re managing Counter’s lifecycle with ChangeNotifierProvider.

This removes the periodic Timer. Not only does that simplify the example and makes it closer to the original, it also prevents a leaking timer (though, in this case, it’s not an issue, since the timer is needed for the whole duration of the app). I experimented with a more robust approach (having or injecting a Timer/StreamController into the Counter, and disposing of it there) but that seemed overly complex for such a simple example. This whole problem will get significantly easier with rrousselGit/provider#46, at which point I could reintroduce this. I will also think about a more complex Provider example, something like the venerable `bloc_complex`, with infinite-scrolling pagination and all that.
@shamilovtim
Copy link

Hey guys,

Something unclear about the docs. I am beginning to see that from the example the valueNotifier in ProxyProvider is coming from the Provider above it. However the docs mention nothing about this and the variables aren't named the same, there's no indication that the constructor or object instance is shared. The parameter is passed in lower camel case but it's not clear where it's coming from. It's completely confusing. Can we update the docs to explain what's going on here?

joshpeterson30489 added a commit to joshpeterson30489/flutter_maps_firestore that referenced this issue Sep 30, 2022
After this change, we’re managing Counter’s lifecycle with ChangeNotifierProvider.

This removes the periodic Timer. Not only does that simplify the example and makes it closer to the original, it also prevents a leaking timer (though, in this case, it’s not an issue, since the timer is needed for the whole duration of the app). I experimented with a more robust approach (having or injecting a Timer/StreamController into the Counter, and disposing of it there) but that seemed overly complex for such a simple example. This whole problem will get significantly easier with rrousselGit/provider#46, at which point I could reintroduce this. I will also think about a more complex Provider example, something like the venerable `bloc_complex`, with infinite-scrolling pagination and all that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants