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

bloc as an extension for Provider #416

Closed
timrijckaert opened this issue Jul 18, 2019 · 9 comments
Closed

bloc as an extension for Provider #416

timrijckaert opened this issue Jul 18, 2019 · 9 comments
Assignees
Labels
discussion Open discussion for a specific topic

Comments

@timrijckaert
Copy link

timrijckaert commented Jul 18, 2019

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 version 0.19.0 it now includes the Provider 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 the Provider package.
We are not using the BlocProvider since it does not have this concept of ProxyProvider which we like.
We consume the events emitted with the use of the BlocBuilder and the BlocListener 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 the Provider 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

@basketball-ico
Copy link

@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.

@timrijckaert
Copy link
Author

timrijckaert commented Jul 18, 2019

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.)

@axellebot
Copy link
Contributor

axellebot commented Jul 18, 2019

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).

@felangel
Copy link
Owner

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 👍

@felangel felangel self-assigned this Jul 18, 2019
@felangel felangel added the discussion Open discussion for a specific topic label Jul 18, 2019
@timrijckaert
Copy link
Author

timrijckaert commented Jul 20, 2019

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 ProxyProvidersyntax.

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 the ProxyProvider 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
    • Too analogous to vanilla Provider syntax
    • I don't like the naming of repository since it such a weird/confusing term to use for non-repository dependencies. Ref: AnalyticsService, DeeplinkHelper, ...
    • No syntactic sugar you get with the ProxyProvider
  • MultiBlocProvider
    • completely interchangeable with 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

@felangel
Copy link
Owner

felangel commented Jul 20, 2019

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?

@timrijckaert
Copy link
Author

timrijckaert commented Jul 21, 2019

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 the RepositoryProvider 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 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
[...]

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.

@felangel
Copy link
Owner

@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 👍

@timrijckaert
Copy link
Author

Thanks @felangel
Implemented the changes today!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Open discussion for a specific topic
Projects
None yet
Development

No branches or pull requests

4 participants