felangel / bloc

A predictable state management library that helps implement the BLoC design pattern
https://bloclibrary.dev
MIT License
11.72k stars 3.38k forks source link

Builder reacting only to subset of the state #174

Closed OndrejKunc closed 5 years ago

OndrejKunc commented 5 years ago

Is your feature request related to a problem? Please describe. Let's say I have a form with quite complex state in my bloc and multiple widgets depend on different parts of the state. If I use BlocBuilder and change the state then all the widgets must be rebuild even if they do not depend on any changed data from the state.

Describe the solution you'd like My solution would be a widget similar to the BlocBuilder but it would also accept converter parameter:

typedef StateConverter<S, ViewModel> = ViewModel Function(S state);

The usage would look like this:

BlocProjectionBuilder(
 bloc: bloc,
 converter: (state) => state.myProperty,
 builder: (context, viewModel) {
   ...
  },
 )

This is a similar concept to StoreConnector in flutter_redux library. Main benefit is that the widget will be rebuild only when data from the converter state projection has changed.

Describe alternatives you've considered An alternative to this approach can be to simply define projection streams in the user blocs or in the widget: myStream = state.map((s) => s.myProperty).distinct(); The stream can be consumed using standard StreamBuilder. I find my approach less verbose though.

Implementation details I can think of two possible implementations. It can be achieved either by modifying existing BlocBuilder or by introducing something like BlocProjectionBuilder. Problem with first approach is that it will make this widget more complex with additional generic parameter. In case of the second option it can share the same base class with BlocBuilder and the BlocBuilderBase will have to be extended with the optional projection logic.

if you think this makes sense to be included as a part of the library I can make a pull request. I have already implemented it in my project.

felangel commented 5 years ago

Hey @OndrejKunc 👋

Thanks for putting this together! I've been thinking about this for some time now and it's awesome that you've already put together your own implementation 💯

Would it be possible for you to share an example project with your implementation?

OndrejKunc commented 5 years ago

Awesome! I made a quick dirty example in my fork https://github.com/OndrejKunc/bloc/blob/bloc-projection-builder/examples/flutter_form/lib/login/login_form.dart

And here is the first BlocProjectionBuilder implementation attempt: https://github.com/OndrejKunc/bloc/blob/bloc-projection-builder/packages/flutter_bloc/lib/src/bloc_builder.dart

felangel commented 5 years ago

Perfect! I’ll take a look and discuss this feature with the team over the next few days. Thanks so much for putting this together 🙏

felangel commented 5 years ago

@OndrejKunc After thinking about this some more and discussing it with the team, I'd like to better understand the use-case for this new feature. Several aspects of this functionality that concern me are:

Overall, I'm still not completely convinced one way or another but I'm leaning toward not including this in the bloc library (for now) unless we have a very solid use-case that can't easily be solved with the existing functionality/APIs. Let me know what your thoughts are and I really appreciate all of the thought and effort you put into this 👍

OndrejKunc commented 5 years ago

@felangel I must admit that the form example is not the perfect use case since rebuilds are called automatically on events likes focus changed as you pointed out.

My general concern was the situation, where you have some data on the bloc that change very often and other data that change less often and for some reason it makes sense to have both of those data in one bloc.

Another example I can think of right now is some kind of a stop watch screen. You would want to show minutes, seconds and milliseconds in the real time and you decided that all of those parts will be in a separate fancy widgets. Then I think it makes sense to keep all the time information in a single bloc. The bloc will generate new state each millisecond, so your millisecond widget can update. Unfortunately your seconds widget and minute widget will have to be rebuilt as well. In this case I think using my solution can have some performance impact.

With that being said I must also agree with all the points you listed. In most cases it will be just over optimization and it can be easily misused by the users.

But the fact is that this is just another builder which can be provided on the top of the library without its modification. So I'm thinking I can maybe roll out my own package which would contain just this builder and will depend on your library if you don't mind. This way anyone who would be interested in this approach would just install another dependency.

Also thanks for looking into this and giving it so much thought.

felangel commented 5 years ago

@OndrejKunc I think you rolling this out as a separate package would be awesome! In my opinion, that's the best solution because we get the best of both worlds 😄

Thanks again for this awesome discussion and for all the work you're doing to help the community 👍

Closing this but looking forward to seeing your package on pub!

OndrejKunc commented 5 years ago

@felangel just FYI here is the package on pub https://pub.dartlang.org/packages/flutter_bloc_extensions :)

felangel commented 5 years ago

@OndrejKunc that's awesome! I added it to the docs 💯

OndrejKunc commented 5 years ago

@felangel Wow, thanks for the great promotion! 👍
I hope I will be able to keep the quality of the package on a decent level, otherwise feel free to remove it from the docs and readme anytime 😃

CosmicPangolin commented 5 years ago

@felangel @OndrejKunc

I'm in the process of porting ~50k lines of a complex (and increasing) app project from a thin redux-like implementation to Bloc. To me, mapping Blocs loosely to widgets feels far more architecturally sound and intuitive than dispatching/composing/side-effect-ing actions to manage a global state. I find this pattern more 'Fluttery,' and far more approachable both as a developer and as someone teaching patterns to new hires. With approachability in mind, I think a substate mapping should absolutely be core:

1) Boilerplate matters a lot wrt cognitive overhead - it's a huge advantage that Bloc has over Redux. But my app has a rich state, with the potential for lots of data to update in quick succession. Without the ability to listen in to a Bloc substate, I already have a handful of cases where I would be forced to write numerous Blocs for a single 'sufficiently large' widget, significantly increasing the amount of stream-y boilerplate to for no reason.

2) It doesn't really seem like a state management library should be making me choose between a preferred (narrowest-possible) Bloc scope or poorly optimized widgets. I don't think you can trivialize optimization; Flutter is fast, but the render times and computations are not zero...especially with complex, animated widgets. Rebuilds can also be CPU (and thus battery) intensive. It's easy to imaigne custom build logic that make rebuild optimization preferred or necessary. All of these things are relevant to my app, but they already matter in something as simple as the stopwatch example https://medium.freecodecamp.org/how-fast-is-flutter-i-built-a-stopwatch-app-to-find-out-9956fa0e40bd

3) This isn't really API complicating so much as the state-of-the-art across state management libraries...it should be familiar to the majority of UI developers, and better leverages the power of streams. It makes it easier for a developer with Redux experience to transition while using familiar functionality

So I guess in summary, I generally believe the Bloc scope should be left to the developer - and wide scope should absolutely not reduce performance in the absence of a helper library. I'm happy to use an extension library...but I really really think it makes sense to put this feature in core.

Thanks for the hard work! It's going to good use :)

felangel commented 5 years ago

@CosmicPangolin thanks for giving the bloc library a try and for the awesome feedback! Regarding your questions:

  1. Can you please give us a specific example/use-case that you have which illustrates this? I think it would be really helpful to gain some more perspective.

  2. In general, my intention was to create an abstraction that allows developers to separate business logic into small, focused units. If you start to stray from that and design for a single bloc that contains the entire application state (in the most extreme case), then I feel that we are teetering on the verge of redux and might be better off just using flutter_redux instead. I would love to hear your thoughts on this. Just to be clear, I am not trying to say don't optimize your code; I am just asking the question: is this the right way to optimize the code? I would argue that if you are representing application state in such a way where different parts of the application react only to subsections of that state, then you should consider splitting the state into smaller states.

  3. Just for my understanding, when you reference state-of-the-art state management what libraries are you referring to? It would really help me understand what libraries you're used to and are using as references. In Redux, for example, it makes sense for this functionality to exist as you only have a single application state which different parts of the application are reacting to/consuming. The difference is, in the bloc library we are distributing the state to specialized blocs. As I mentioned in the first point, it would be super useful to get some insight into some use-cases that you have. That way, we can have a concrete problem to solve and keep the discussion a lot more focused.

I really appreciate this thorough feedback and am looking forward to continuing this discussion and improving the library so that we can make everyone's lives easier :)

Thanks again! 💯

CosmicPangolin commented 5 years ago

@felangel Sure, but bear with me cause this will take a bit of context to illustrate. Consider the following structure:


main.dart
<app>
     material_app.dart
     <src>
          scaffold.dart
          scaffold_bloc
          <chats>
               chats.dart
               chats_bloc
               chats_widgets
                    entity_collection_stats.dart
                    super_beautiful_fancy_entity_header.dart
               <chat>
                    chat.dart
                    chat_bloc
                    chat_widgets
                    <chat_message>
                         chat_message
                         chat_message_bloc
                         chat_message_widgets
          <friends>
          <groups>

....and so on. The nested layout allows us to define blocs and widgets that are in scope with both an interface module and a layer of data (chats_bloc & chats_widgets in scope with chats.dart, which actually builds a widget representation of all chats - an interface with a chat list and aggregate metadata). Now let's assume there can be a chat list for collections of groups or collections of friends. We have two widgets in scope with chats.dart which need notification when the collection changes for interface rebuilds: super_beautiful_fancy_entity_header (single rebuild), and entity_collection_stats (rebuild on any element update within the collection). These two widgets can have very different rebuild rates, but rely on the same fundamental data streamed through chats_bloc: a single collection of chat metadata. Additionally, we have a sub-module (chat.dart) that depends on a single entity's metadata.

We have ~3 options for building with the current Bloc library (annotated with my feelings on each):

  1. Update both entity_header_widget and aggregate_entity_stats on every element update.
    • Really bad. Obviously sub-optimal, and while things would need to get pretty fancy to cause jank, you can easily be running a lot of unnecessary CPU cycles - imagine super_fancy is a Widget function that returns different configurations depending on initial collection stats, so has its own set of non-trivial computations

2) Limit chats_bloc to stream only changes in the collection type, and push element updates down into a new entity_collection_stats_bloc.

3) Pull entity_header_widget up (or pass a variable down), using some higher-level function or bloc event to determine the collection type

Even though 3 works (and reflects most app trees that I see, with some variable passing), I actually think the cleanest, most architecturally sound (read: modular) solution is none of the above - rather, fully determine the collection type in chats_bloc and provide that type through a provider in the chats.dart widget function, but provide the collection for element updates in entity_collection_stats. This way you have two variables in the chats_bloc - the collection and a variable representing its type - that are disseminated to the appropriate sub-widgets, but they're scoped appropriately for that layer of data :)

I like this option a lot. It maps very well to both NoSQL database structure and widget trees, couples business logic with the relevant scope (thus minimizing boilerplate for each layer of data), and enforces modularity. Every other option breaks at least one of those principles. This is why I have such strong feelings on the matter - I think it enables a better architecture, from an organization and scalability perspective.

happapa commented 5 years ago

Would a more concrete example of this be a simple counter app, like the default flutter project? A class called CounterBloc would have the current counter value (int) and information whether the value is divisible by 1000000 (bool) as its state. The int value will change very often and the bool very rarely. Both things are tightly connected, so it wouldn't make sense to me to try to divide the bloc into smaller components. The UI has one text widget that shows the counter value and updates often. Another widget changes the background color based on the bool state and updates very rarely. Both widgets are listening to the CounterBloc which results in many unneccessary rebuilds for the background color.

tonisostrat commented 5 years ago

I think this article has a very apt illustration of the problem discussed in this thread. CTRL+F BloC Pattern and just read through that part.

felangel commented 5 years ago

Moving this to #315. I would love to hear everyone's thoughts 👍

uberchilly commented 4 years ago

With that being said I must also agree with all the points you listed. In most cases, it will be just over-optimization and it can be easily misused by the users.

But isn't it described in performance best practices here https://flutter.dev/docs/perf/rendering/best-practices to avoid calling setState too high in the hierarchy. "Avoid calling setState() high up in the tree if the change is contained to a small part of the tree." It looks to me that by pushing whole states over and over bloc will affect UI to updated from higher up in the hierarchy resulting in unnecessary updates for many views that are still the same which is contrary to the guidelines (not sure what flutter does under the hood when the view is exactly the same like it was in the last frame). But if we split the problem into multiple blocs to get separate rebuilds we lose the ability of data to be connected in one bloc.

On the other hand, if we use bloc with state being separated with multiple streams that can be hooked to the exact part of the view hierarchy that we want to update, we lose the ability like undo/redo and history of states.

narcodico commented 4 years ago

Hi @uberchilly 👋

There's an optional condition on the BlocBuilder which you can use to control when exactly to rebuild a sub-tree based on a piece of your state. Keep in mind this doesn't prevent flutter from rebuilding it if it's needed.

uberchilly commented 4 years ago

@RollyPeres Thanks for the answer, I have found about that just a few minutes after I've posted this. I guess that solves it then.