Closed timrijckaert closed 5 years ago
@timrijckaert I don't like this idea, currently flutter_bloc
use provider
and expose providers (BlocProvider, RepositoryProvider) with syntax sugar for bloc library users, so this reduce boilerplate code if your are using bloc packages.
Also respect ProxyProvider
this is not a valid widget for BLoC pattern, because when you combine 2 blocs is business logic, so you can create a bloc that have dependency of other blocs, in this way now you can share this code with other platforms like AngularDart
, and in flutter you can listen the state of this new bloc with the BlocBuilder
.
Hmm I think you misunderstood my intention.
We use the ProxyProvider
class for injecting dependencies into a BLoC
not to combine multiple BLoC
's.
Anyway that's more or less besides the point I'm trying to make.
RepositoryProvider
could be accomplished with the vanilla Provider
package. (Just look at the implementation.)
Hey @timrijckaert I do understand your thought.
Keeping the MultiBlocProvider
, MultiBlocListener
and (Multi)RepositoryProvider
allows us to have a fully shipped working library without depending on customized Widget creation (e.g. InheritedWidget
for injection) or other libs dependency (like provider
).
But indeed if you are using Provider
from provider
for other purposes you can use it to also "inject" the blocs and not using BlocProvider
at all. (same for others extra tools).
Hi @timrijckaert 👋 Thanks for opening an issue!
I would love to hear more about your experience with flutter_bloc
and why you are using provider
directly instead of BlocProvider
and RepositoryProvider
. What's the use-case for using ProxyProvider
with flutter_bloc
?
provider
is an unopinionated, generic DI library and as others have pointed out with flutter_bloc
we have the opportunity to provide specialized widgets which help specifically with the bloc library.
In general, I believe it's a better developer experience to be able to import flutter_bloc
and simply get started without having to add a separate DI dependency and establish your own patterns.
In addition, having a layer of abstraction between provider
and the flutter_bloc
API is critical because it will allow us to have a stable API regardless of changes in provider
all while still benefiting from improvements and newly added features in provider
.
Lastly, I want to mention that MultiProvider
and all other other Multi
widgets have nothing to do with provider
and are simply nested widgets. I am working with Remi to extract that into a separate package. Once that work is done both provider
and flutter_bloc
will be able to add nested
as a dependency.
Looking forward to hearing your thoughts! Thanks 👍
Hi @felangel and everybody else who commented.
Thanks for the quick reply.
Let me quickly give you a summary of how we setup our ioc
in our app.
Currently we have different scoping
levels setup within our app.
We have an AppScope
which provides essentially singletons throughout the app.
Some of these dependencies don't fit semantically in the naming of a RepositoryProvider
.
This includes a DeeplinkHelper
, DomainHelper
, AnalyticsService
, Network Service
, Db Service
, NavigationStrategyHelper
, ...
We also provide an AppBloc
but use the ProxyProvider
since it needs multiple other dependencies, like I said before.
ProxyProvider6<Dio, UserStorage, VRTThemeBuilder, VRTNavigator, DeepLinkHelper, CrashReportingHelper, AppBloc>(
dispose: (context, val) => val.dispose(),
builder: (
BuildContext context,
Dio dio,
UserStorage userStorage,
VRTThemeBuilder vrtThemeBuilder,
VRTNavigator vrtNavigator,
DeepLinkHelper deepLinkHelper,
CrashReportingHelper crashReportHelper,
AppBloc previous,
) =>
AppBloc(
tabBarChoices.first,
dio,
_defaultEnv,
userStorage,
vrtThemeBuilder,
vrtNavigator,
deepLinkHelper,
crashReportHelper,
),
),
Besides an AppScope
we also have a scope for each logical screen: Screen1Scope
, Screen2Scope
.
Each scope
has multiple dependencies
which we provide with the handy MultiProvider
widget.
We provide these via the standard Provider.value
syntax.
Screen1Scope
and Screen2Scope
mostly depend on higher level dependencies therefore again we use the ProxyProvider
syntax.
MultiProvider(
providers: [
ProxyProvider2<Nw, Local, HeadlineRepository>(
builder: (context, nw, local, prevRepo) => HeadlineRepository(nw, local),
),
ProxyProvider<HeadlineRepository, HeadlineBloc>(
updateShouldNotify: (prev, curr) => false,
builder: (context, headlineRepo, prevBloc) => prevBloc ?? HeadlineBloc(headlineRepo),
dispose: (context, bloc) => bloc.dispose(),
)
],
child: ...,
);
Note that I could have used the
RepositoryProvider
here, but theProxyProvider
provides a nicer API.
we have the opportunity to provide specialized widgets which help specifically with the bloc library.
I agree and therefore I feel that since flutter_bloc
now relies on the provider
package I don't see any use for:
RepositoryProvider
/MultiRepositoryProvider
Provider
syntaxAnalyticsService
, DeeplinkHelper
, ...ProxyProvider
MultiBlocProvider
MultiProvider
, but if I understand correctly you are already on this.In general, I believe it's a better developer experience to be able to import flutter_bloc and simply get started without having to add a separate DI dependency and establish your own patterns.
Isn't that already the case? 🤔
If I include the flutter_bloc
in my pubspec
I transitively pull in provider
?
Thanks
Hey @timrijckaert thanks for the additional info!
Currently we have different scoping levels setup within our app. We have an AppScope which provides essentially singletons throughout the app. Some of these dependencies don't fit semantically in the naming of a RepositoryProvider. This includes a DeeplinkHelper, DomainHelper, AnalyticsService, Network Service, Db Service, NavigationStrategyHelper, ... We also provide an AppBloc but use the ProxyProvider since it needs multiple other dependencies, like I said before.
Correct me if I'm wrong but isn't ProxyProvider only necessary if the dependencies change? I'm not sure I understand why it's necessary as dependencies such as Dio, UserStorage, etc... should probably not change throughout the application lifecycle.
Even though this architecture may work for you, it's not what I would recommend because you end up having things like the http, storage, and db clients accessible from anywhere in the widget tree which makes it very easy to couple your UI with these low level data providers. Instead, I would recommend creating and providing your repositories using the RepositoryProvider and then accessing your repositories in the various parts of your widget tree in order to build the necessary blocs. Since the repositories and their dependencies should not change I don't see a need for ProxyProvider with this approach.
Besides an AppScope we also have a scope for each logical screen: Screen1Scope, Screen2Scope. Each scope has multiple dependencies which we provide with the handy MultiProvider widget. We provide these via the standard Provider.value syntax.
When you strictly use the Provider syntax, under the hood provider uses inheritFromWidgetOfExactType
to do the lookup which will cause more rebuilds because the widget will rebuild whenever changes occur to the provided widget or its ancestors.
Instead, the BlocProvider
and RepositoryProviders
override the default provider behavior to use ancestorInheritedElementForWidgetOfExactType
since with this architecture you should not have blocs and repositories dynamically changing at runtime.
I don't like the naming of repository since it such a weird/confusing term to use for non-repository dependencies. Ref: AnalyticsService, DeeplinkHelper, ...
You're right that you shouldn't use RepositoryProvider
to provide an AnalyticsService
to your widget tree and I would argue you likely don't need to provide your AnalyticsService
at all because it can just be directly injected into your repositories. Also, you can use BlocDelegate
for all bloc related analytics.
At the end of the day, if this architecture works well for you then definitely keep it up but I personally think most of your observations/critiques stem from the fact that your architecture diverges from bloc library architecture.
If I include the flutter_bloc in my pubspec I transitively pull in provider?
Yes but it's bad practice to directly use a transitive dependency because you're assuming that transitive dependency will always be there.
Thoughts?
Ok, this helps to clear up some decisions made in this library for me.
I'm very new to Flutter so these kind of discussions help me to get some feedback from more experienced Flutter/Dart devs.
Our architecture of our app is still in an early stage so we could use some guidance on best practices.
Just some thoughts/questions:
Correct me if I'm wrong but isn't ProxyProvider only necessary if the dependencies change?
According to the provider
documentation it indeed states, like you said:
ProxyProvider is a provider that combines multiple values from other providers into a new object, and sends the result to Provider.
That new object will then be updated whenever one of the providers it depends on updates.
I don't know if that is the only valid reason to use it, I just really like the syntax of using type params to get a certain dependency?
Maybe Remi Rousselet could give some clearance on that?
Coming from an Android background I thought it was similar to how Dagger works, were it injects previously declared dependencies, instead of you manually getting them with the Provider.of
syntax.
I'm not sure I understand why it's necessary as dependencies such as
Dio
,UserStorage
[...] I would recommend creating and providing your repositories using theRepositoryProvider
and then accessing your repositories in the various parts of your widget tree in order to build the necessary blocs
The reason we provide these lower level objects at the root of our Widget tree is so we can have the scoping for each screen.
I don't like the idea of having to declare Screen1Repository
, Screen2Repository
in my AppScope
, effectively creating singletons for each of my screens at startup time.
Just to encapsulate my lower level objects.
We do PR's and would off course not approve if someone were to use the Nw
object as is.
Right now we only construct the ScreenRepositories
whenever a user navigates to this specific part of the Widget tree and gets destroyed afterwards when the user leaves that screen.
When you strictly use the Provider syntax, under the hood
provider
usesinheritFromWidgetOfExactType
to do the lookup which will cause more rebuilds because the widget will rebuild whenever changes occur to the provided widget or its ancestors [...]
I don't know if I understand this correctly, could you perhaps have a little sample where the different behaviours are demonstrated? Cause I believe we are not changing the dependencies once they are made?
Instead, the BlocProvider and RepositoryProviders override the default provider behavior to use ancestorInheritedElementForWidgetOfExactType
Same as Provider.of(context, false)
right?
I would argue you likely don't need to provide your AnalyticsService at all because it can just be directly injected into your repositories. Also, you can use BlocDelegate for all bloc related analytics.
True.
But did you mean 'injected into your BLoC
's', right?
We could make the AnalyticsService
only be accessible within the BlocDelegate
and only let it react when a certain Event
was emitted by the AppBloc
.
While there is definitely a case to be made for the AnalyticsService
sometimes you want to declare top level dependencies which are not necessarily a Repository
right?
How would you go about this, or does every little dependency need to be encapsulated by either a BLoC
or a Repository
?
For example we have this ImageProfileSelector
inside of our AppScope
which is basically a helper class that selects the correct Thumbor profile based on the size of an Image Widget.
We therefore have a custom Picture
widget which tries gets the ImageProfileSelector
.
Therefore not routing through a BLoC
first.
Is this wrong?
Thanks in advance for the many questions.
@timrijckaert glad I was able to help!
I don't know if that is the only valid reason to use it, I just really like the syntax of using type params to get a certain dependency? Maybe Remi Rousselet could give some clearance on that? Coming from an Android background I thought it was similar to how Dagger works, were it injects previously declared dependencies, instead of you manually getting them with the Provider.of syntax.
You can definitely use it like that but I don't think it's the most efficient implementation if your dependencies don't ever change. The implementation calls Provider.of<T>(context)
for each dependency which seems unnecessary in your case.
The reason we provide these lower level objects at the root of our Widget tree is so we can have the scoping for each screen. I don't like the idea of having to declare Screen1Repository, Screen2Repository in my AppScope, effectively creating singletons for each of my screens at startup time. Just to encapsulate my lower level objects. We do PR's and would off course not approve if someone were to use the Nw object as is.
What I would recommend is not have "ScreenRepositories" but instead think of your repositories as your domain layer. You might have a UserRepository
, TodoRepository
, etc... which have all the domain specific logic without the feature/application logic. Those repositories would be provided at root of the application and then you would have feature-specific blocs such as a AuthenticationBloc
, TodosBloc
, etc... which interact with the repositories and drive the application state. You would never be passing around http clients or db clients but instead you'd create BlocProviders that use RepositoryProvider to inject the appropriate repositories into the bloc. Then your UI layer is purely reacting to the bloc state via BlocBuilder and updating the state via dispatch.
I don't know if I understand this correctly, could you perhaps have a little sample where the different behaviours are demonstrated? Cause I believe we are not changing the dependencies once they are made?
If you set listen
to true then you are telling flutter rebuild me whenever the thing you're listening to rebuilds or if its ancestors rebuild. That results in many more rebuilds of your widget for no reason in this case.
Same as Provider.of(context, false) right?
Yes except that is not the default behavior of provider so it's just one example of why I don't want to let developers have to figure that out on their own and instead just make those decisions in the library since it doesn't make sense to use listen: true in the context of the bloc library architecture (the only thing changing is the data flowing through the streams).
But you mean directly injected into your BLoC's right?
If you want to have bloc-specific analytics, then yes. Otherwise, I'd recommend using BlocDelegate for global analytics and then if you want domain layer analytics you can have the repositories have a dependency on the AnalyticsService
.
Plus while there is definitely a case to be made for the AnalyticsService sometimes you want to declare top level dependencies which are not necessarily a Repository right? How would you go about this, or does every little dependency need to be encapsulated by a either a BLoC or a Repository ?
I would argue that for most cases you should not be passing around top level dependencies and instead you should have the repositories handle the domain layer (interacting with data providers like apis and dbs) and then have the bloc layer handle the application/feature logic. This keeps your architecture clean, making each part reusable.
For example we have this ImageProfileSelector inside of our AppScope which is basically a helper class that selects the correct Thumbor profile based on the size of an Image Widget. We therefore have a custom Picture widget which tries gets the ImageProfileSelector. Therefore not routing through a BLoC first. Is this wrong?
I would personally have just made the ImageProfileSelector
a widget that can be used in the widget tree instead of a dependency that you pass around via BuildContext
since it's a presentational component.
Thanks for the awesome discussion! Let me know if that helps further clarify things 👍
Thanks @felangel Implemented the changes today!
Is your feature request related to a problem? Please describe. Fist of all thank you for the open discussion (#354 ) about incorporating the
Provider
package inside this package. With the arrival of version0.19.0
it now includes theProvider
package.I believe this is a step in the right direction for this package.
Still, I would have like to see more overlapping functionality removed from this package.
In our app we are injecting our
BLoC
's solely wîth the use of theProvider
package. We are not using theBlocProvider
since it does not have this concept ofProxyProvider
which we like. We consume the events emitted with the use of theBlocBuilder
and theBlocListener
Widgets.Describe the solution you'd like Only keep the
BLoC
specific widgets inside this package eg:BlocProvider
BlocBuilder
BlocListener
I think all of the other Widgets have an equivalent inside the
Provider
package.If not please correct me.
Describe alternatives you've considered We are currently using both packages side by side and that works just fine, however it feels like we have two packages trying to accomplish too much of the same thing. Since Google more or less encourages the use of
Provider
as the sort of default inversion of control package, I would like to see this package as an EXTENSION of theProvider
package.Additional context Thanks for your massive investment in the Flutter community.
I hope forward for a healthy open minded discussion.
Thanks in advance
Tim