felangel / bloc

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

[Feature Request] MultiBlocBuilder #538

Closed PhilipChng closed 3 years ago

PhilipChng commented 4 years ago

Sometimes I would need to access different blocs in the same widget, and the code goes pretty messy. I am thinking maybe we can have something like MultiBlocBuilder which serve the similar purpose of MultiRepositoryProvider and MultiBlocProvider.

felangel commented 4 years ago

Hi @pczn0327 πŸ‘‹ Thanks for opening an issue!

This has been brought up in the past but we have decided against it for several reasons.

Can you please provide some use-cases? If there are valid use-cases and enough people would benefit from it, then I'm open to including it. Thanks πŸ‘

PhilipChng commented 4 years ago

Hi @felangel , I can't tell you the exact use case, but I've simulated similar use case in this repo.

Use case usage comes from different form is rendered based on input from user.

I hope that helps.

henriquearthur commented 4 years ago

I'm facing the same problem on my project. I believe only apps from medium-large complexity faces this.

From @pczn0327 example:

          Expanded(
            child: BlocBuilder<BlueBloc, bool>(
              bloc: blueBloc,
              builder: (context, blueState) {
                return BlocBuilder<GreenBloc, bool>(
                  bloc: greenBloc,
                  builder: (context, greenState) {
                    return BlocBuilder<RedBloc, bool>(
                      bloc: redBloc,
                      builder: (context, redState) {

This can get worse when we have to do a few ifs to check for current state. A MultiBlocBuilder would make the code more readable.

In the meantime, or in case MultiBlocBuilder doesn't get implemented, do you recommend any approach when using multiple blocs like this @felangel?

felangel commented 4 years ago

@henriquearthur yup in the meantime you need to nest BlocBuilders just as you've shown πŸ‘

I'm putting together an implementation that will expose BlocBuilder2-BlocBuilder9 which can be used like:

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(title: Text('Example')),
      body: BlocBuilder2<BlueBloc, BlueState, GreenBloc, GreenState>(
        builder: (context, blueState, greenState) { ... },
      ),
    );
  }

Please let me know what you think of the proposed api πŸ™

PhilipChng commented 4 years ago

Hi @felangel , does that means if I need to use 5 blocs, I will need to do as follow? BlocBuilder5<BlueBloc, BlueState, GreenBloc, GreenState, RedBloc, RedState, WhiteBloc, WhiteState, BlackBloc, BlackState>?

felangel commented 4 years ago

@pczn0327 yup or alternatively you will need to manually provide the blocs like:

BlocBuilder5(
  blocA: blueBloc,
  blocB: greenBloc,
  blocC: redBloc,
  blocD: whiteBloc,
  blocE: blackBloc,
  builder: (context, blueState, greenState, redState, whiteState, blackState) { ... }
)

Without that Dart will not be able to infer the state types and all states will resolve to type dynamic.

PhilipChng commented 4 years ago

Cool! Sounds like a good start for it! Looking forward for the solution!

adityadroid commented 4 years ago

This will definitely be useful. Infact the first time I came across a usecase where I had to nest multiple BlocBuilders, I went through the entire documentation of bloc to see if there was a cleaner way of doing it. Nesting multiple BlocBuilder Widgets somehow felt wrong to me.

ThinkDigitalSoftware commented 4 years ago

I think 9 is too many. At that point code should be refactored. If course we have to account for edge cases, but in that case, there's no end. Imagine what the code for that widget is going to look like!

andredsnogueira commented 4 years ago

It would be awesome.

felangel commented 4 years ago

I think 9 is too many. At that point code should be refactored. If course we have to account for edge cases, but in that case, there's no end. Imagine what the code for that widget is going to look like!

@ThinkDigitalSoftware I completely agree. I was just messing around and I'm still not sold on if this belongs in flutter_bloc because it's not really a "core" component. It's more of a convenience thing which might be better as an independent package.

warriorCoder commented 4 years ago

I'm with Felix on this one. It seems like this isn't part of the core functionality, and I'd also like to see a real live use case. Why would you need to nest BlocBuilders in a real app?

I'm wondering if there isn't a different way to build to the widget (like maybe it is trying to do too many things, or the blocs are too granular)?

ThinkDigitalSoftware commented 4 years ago

You can also combine your streams ahead of time and provide the combined stream to a single bloc. This will simplify your build methods.

ThinkDigitalSoftware commented 4 years ago

I believe that they can also separate the widgets by layers. For example, if they require the authentication state in a widget 5 layers down, that should be part of the state that's passed from above . I agree that their blocs may be too granular . I looked at the op's code, but the question is, why would you have separate voice for information like this?

DeepStyles commented 4 years ago

In this feature, I wouldn't want to rebuild unnecessary having consumed multiple blocs... Provider package has updated with selector widget recently which only rebuild when related Bloc changes. (My widget go crazy building, if one f the multiple blocs provided is related to animation)

felangel commented 4 years ago

@DeepStyles you'd still be able to provide conditions for each bloc like:

@override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(title: Text('Example')),
      body: BlocBuilder2<BlueBloc, BlueState, GreenBloc, GreenState>(
        conditionA: (prevBlueState, nextBlueState) => ...,
        conditionB: (prevGreenState, nextGreenState) => ..., 
        builder: (context, blueState, greenState) { ... },
      ),
    );
  }

Either way I'm leaning towards not moving forward with this feature because I don't see many use-cases and nested BlocBuilders are more readable/maintainable in my opinion. Also, as many people have pointed out if you have lots of nested BlocBuilders there's a good chance you need to refactor your blocs.

felangel commented 4 years ago

@henriquearthur I took a closer look at your use-case and I would argue that you can refactor to just have a ColorBloc rather than a RedBloc, GreenBloc, and BlueBloc. You can have a ColorChanged event which takes r, g, and b values and a ColorState which keeps track of the current r, g, and b value. I don't think you would need a MultiBlocBuilder anymore at that point.

felangel commented 4 years ago

Closing this for now because I have not seen a compelling use case for this. In addition, I want to keep the library as simple as possible and I feel adding these widgets just adds unnecessary complexity. If anyone feels like they have a good use case please let me know and I'm happy to re open it πŸ‘

ThinkDigitalSoftware commented 4 years ago

@felangel nitpicking, but your label wontfix conveys that this topic is closed for future discussion

felangel commented 4 years ago

@ThinkDigitalSoftware sorry! I removed won't fix and re-added discussion/feedback wanted πŸ‘

MichelMelhem commented 4 years ago

In the case of a complex view that use multiple source of data, I think that the widget you mentionned above would be very useful.

felangel commented 4 years ago

@MichelMelhem can you give an example?

algirdasmac commented 4 years ago

Hello,

I was watching this feature request and I think I have one use case, where this MultiBlocBuilder would be usefull. Lets look at this example:

class MultiBlocExampleWidget extends StatefulWidget {
  const MultiBlocExampleWidget({Key key}) : super(key: key);

  @override
  _MultiBlocExampleWidgetState createState() => _MultiBlocExampleWidgetState();
}

class _MultiBlocExampleWidgetState extends State<MultiBlocExampleWidget> {
  final int totalSteps = 5;
  int selectedStep = 0;

  @override
  Widget build(BuildContext context) {
    return Stepper(
      currentStep: selectedStep,
      onStepCancel: () {
        if (selectedStep > 0) {
          setState(() => selectedStep = selectedStep - 1);
        }
      },
      onStepContinue: () {
        if (selectedStep < totalSteps - 1) {
          setState(() => selectedStep = selectedStep + 1);
        }
      },
      onStepTapped: (index) => setState(() => selectedStep = index),
      steps: [
        Step(
          title: Text('Address'),
          subtitle: Text('selectedAddress from AddressBloc'),
          content: Text('address list from AddressBloc'),
        ),
        Step(
          title: Text('Delivery time'),
          subtitle: Text('selectedDeliveryOption from DeliveryBloc'),
          content: Text('delivery options from DeliveryBloc'),
        ),
        Step(
          title: Text('Payment method'),
          subtitle: Text('selectedPaymentMethod from PaymentBloc'),
          content: Text('payment options from PaymentBloc'),
        ),
        Step(
          title: Text('Phone number'),
          subtitle: Text('phoneNumber from UserBloc'),
          content: Text('edit phone number with UserBloc'),
        ),
        Step(
          title: Text('Confirm order'),
          subtitle: Text('orderTotal from OrderBloc'),
          content: Text('order totals info'),
        ),
      ],
    );
  }
}

So I would like to have each step have its own BLoC:

  1. Selecting your delivery address (address bloc)
  2. Selecting delivery time (delivery bloc)
  3. Selecting payment type (payment bloc)
  4. Selecting phone number (user bloc)
  5. Confirming order (order bloc)

So in my case I have to nest all BlocBuilders above Stepper widget, just because Stepper widget parameter steps accepts only List<Step>. Same goes for ExpansionPanelList widget and I think there is more. Also these five blocs merged into one big BLoC would be a real mess (tried that), because they are used also in other parts of the app, just for their own functionality.

This is my real world example that I had to deal with - using BlocBuilder in Flutter widgets, where array of widgets can only be of specific type (not List<Widget>).

ThinkDigitalSoftware commented 4 years ago

@algirdasmac but why would you have separate blocs for these? Why not have a single OrdersBloc? Think of a bloc like a manager in a business. Would you have a Manager over getting the customers address and a separate manager for their payment type? Not really. Your blocs can handle multiple events if they extend the same class, so use that to your advantage and keep things simple. Take a look at Redux examples or other examples with this library that yield states using currentState.copyWith functions to yield new States without losing old data. That way you can make a single block that just focuses on managing your orders page

algirdasmac commented 4 years ago

@ThinkDigitalSoftware I separated each Bloc, so I would not have one Bloc with massive amount of Events and State object would be manageable. Look at it this way:

  1. Payments Bloc issues you a new payment card, removes one if you need, selects your preferred payment method, syncs all your stored payment methods and processes payment.
  2. Delivery and Order bloc is actually the same, I just wanted to make my example more dramatic :). Still this bloc is responsible of syncing order info with database, selecting available delivery times, save additional comments.
  3. Address Bloc writes, edits, deletes your addresses, syncs address list with database, also searches for address in google places.
  4. User Bloc is the one, where you set your email, phone number, password, name and etc.

Would you have one manager in the company to issue cards, select delivery time for your order and search for available addresses? Probably not. Each of the bloc have its own page (Payments page, Addresses page, Orders page, User profile page), but this one checkout page brings everyone together. I tried doing everything in one BLoC, but then the code becomes really messy and hard to manage - state object is massive - list of available addresses, selected address, list of payment methods, selected payment method, list of delivery times, selected delivery time, all the user information and etc. Also events are repeated as in smaller blocs, and I needed to copy/maintain same code in two places.

I hope this answers your question.

ThinkDigitalSoftware commented 4 years ago

That can be understandable, but if you had to create an order, would you call all of your managers over to give you an individual piece of that the data to make a single order? It would be more likely that this information was already compiled when you got to this section as a single Order object, which would be compiled by the OrdersBloc. It could be ok to keep this many blocs, but have a shared state or object that you can access from your blocs that would total an Order object so your widget isn't doing the business logic (like assembling the Order). Because anytime you need to access this order, you're going to need to build this same logic again and it'll have to be in the widget tree since that's where you'll have access to the context in order to look up your blocs. It sounded like, from your description of the state object that you had less than 10 fields. Your 6 lists, your User object, etc. If you saw Redux state classes you'd die. This is small compared to what it can get to in Redux. They believe in a single source of truth. One state for the entire app, broken up into smaller classes, etc. So its not an unhealthy design pattern if it's organized properly. But creating more blocs than you need can lead to complication like this. It's up to you, of course. Just wanted to present an alternate way to look at it

algirdasmac commented 4 years ago

There is probably no right way - each has its own πŸ˜„. To be honest, having smaller blocs is really easy to manage, especially when we reuse our logic in web and flutter. Three full time developers are working on BLoC's and it is working fine for us yet, but I am really thankful for your insights and suggestions!

What I tried to do is to show an example, where MultiBlocBuilder would be useful in flutter widget library. Maybe my use case is not the best, but if you need to use two or more blocs for Stepper/ExpansionPanelList it becomes really messy. Still this is very rare use-case.

ThinkDigitalSoftware commented 4 years ago

I hope you find a working solution!

amreniouinnovent commented 4 years ago

@felangel I think we need a better sample for repository builder so we avoid nested bloc builders

tenhobi commented 4 years ago

It would be cool to have some tips how to do complicated things to avoid this problem, I would like to have something to help me improve it. <3

ThinkDigitalSoftware commented 4 years ago

@tenhobi what I've done previously is use a combineLatest function from RxDart to merge my streams and output a stream with a single object that is updated anytime any of the other streams are updated and then subscribe to that using a StreamBuilder

ThinkDigitalSoftware commented 4 years ago

@felangel just came across a use case. In my case, the devs have decided to have a separate bloc per news list tab, which is separated by region. They've added a tab which should show news from all of them so now I need to combine the outputs. This could be tricky with the current nested blocs and conditions. I haven't started reasoning on a solution yet, but this is the first thing I thought of

narcodico commented 4 years ago

@ThinkDigitalSoftware that sounds like you should use a single bloc, filter your data on those tabs and show everything in your latest added tab.

felangel commented 4 years ago

@ThinkDigitalSoftware yeah as @RollyPeres mentioned, have you considered having a single news bloc which you could filter by region or is that out of the question? Would love to learn more πŸ˜„

ThinkDigitalSoftware commented 4 years ago

@ThinkDigitalSoftware that sounds like you should use a single bloc, filter your data on those tabs and show everything in your latest added tab.

That's possible. My issue is that the tech lead doesn't want me spending too much time on what would be considered refinements at the moment.

Here's a question that should be answered though.

When does a library focus on best practices vs freedom of style? This is why pendantic and effective_dart are simply linting suggestions rather than enforced rules. and why var still exists, etc. We should account for the edge cases rather than only adding something when someone can prove it's impossible or less favorable to implement another way. We have a lot of work ahead of us if we're going to police the programming styles of our libraries' users.

I ended up solving this problem by using a CombineLatestStream. I know it's a sin, but it was WAY faster than consolidating 3-4 blocs into one.

felangel commented 4 years ago

Thanks for sharing and for the feedback! I’m a bit curious why that feature was considered a refinement.

We should account for the edge cases rather than only adding something when someone can prove it's impossible or less favorable to implement another way.

Can you elaborate a bit on what you mean by this?

ThinkDigitalSoftware commented 4 years ago

Thanks for sharing and for the feedback! I’m a bit curious why that feature was considered a refinement.

I didn't ask, but based on other things I took the time to fix and was counseled on, this is a bigger task.

We should account for the edge cases rather than only adding something when someone can prove it's impossible or less favorable to implement another way.

Can you elaborate a bit on what you mean by this?

From the viewpoint of a new dev, even if combining blocs isn't a favorable practice, I don't believe it's your business to decide that. Rather than ask why add it, are we able to answer why not? If we have a MultiBlocListener, it seems natural to have its companion that can build. We know it's possible and that there are use cases to listening to multiple blocs. Why not add a builder?

felangel commented 4 years ago

From the viewpoint of a new dev, even if combining blocs isn't a favorable practice, I don't believe it's your business to decide that. Rather than ask why add it, are we able to answer why not? If we have a MultiBlocListener, it seems natural to have its companion that can build. We know it's possible and that there are use cases to listening to multiple blocs. Why not add a builder?

@ThinkDigitalSoftware fair enough. I'll revisit this request this weekend πŸ‘

narcodico commented 4 years ago

@ThinkDigitalSoftware I'm not ruling out the need of combining blocs sometimes, but in my experience, whenever there's a need to build data driven UI's based on more than 2 blocs, it might be a side effect of not properly designing the blocs and their responsibilities.

In my case, the devs have decided to have a separate bloc per news list tab, which is separated by region.

Why not go for a single base bloc which receives a region as a parameter ? This way you guys could have dropped a bloc on top of each tab and have reusable logic. Curious what your thoughts are on this approach @felangel, you endorse it or you consider region parameter as state? πŸ˜ƒ

They've added a tab which should show news from all of them so now I need to combine the outputs.

This kind of scenarios happen a lot, and it's not bloc's fault. They should be taken into consideration from initial design.

ThinkDigitalSoftware commented 4 years ago

@ThinkDigitalSoftware I'm not ruling out the need of combining blocs sometimes, but in my experience, whenever there's a need to build data driven UI's based on more than 2 blocs, it might be a side effect of not properly designing the blocs and their responsibilities.

In my case, the devs have decided to have a separate bloc per news list tab, which is separated by region.

Why not go for a single base bloc which receives a region as a parameter ? This way you guys could have dropped a bloc on top of each tab and have reusable logic. Curious what your thoughts are on this approach @felangel, you endorse it or you consider region parameter as state? πŸ˜ƒ

They've added a tab which should show news from all of them so now I need to combine the outputs.

This kind of scenarios happen a lot, and it's not bloc's fault. They should be taken into consideration from initial design.

You're right in all your replies, but the fact still remains that I can't touch this section of the logic, so I have to work with what I have. Second, there are many MANY ways in which every dev can improve their logic and code. But why cripple them by not providing options and forcing them to refractor? What benefits do we get? This is why so many devs go with provider in their state management solution because it does provide this option and doesn't restrict them to best practices. Someone may surprise us with a valid use case and then we'll say, "Man, I could have added that in the first place".

Again, I understand and agree with the reasons why it should be left out, but it still doesn't mean that it's the best idea. We're programmers. Flexibility is the name of the game.

narcodico commented 4 years ago

@ThinkDigitalSoftware I completely agree that flexibility is important.

What benefits do we get?

From what I've seen so far, bloc is tailored towards enforcing good practices no matter who's writing the code; having someone dropping some fancy opinionated code is harder to do, and that's probably for the best, because it makes for a unified way of writing code. This will make all developers understand it easier and adapt to it way quicker. I believe flexibility comes from how you chose to implement bloc. And each bloc is a Stream so sky is the limit what you can achieve with streams ✌

But why cripple them by not providing options and forcing them to refractor?

combineLatest is a perfectly viable solution if you're trying to ship a quick solution. But I think in long term you get in a way worse spot by not refactoring and that applies to everything code related. If you need to make some workaround to solve an issue, next time you bump into this you'll need to make 3 workarounds, and things will get out of hand rather quickly. This is just a matter of developer perspective I guess(or team lead πŸ˜‰).

This is why so many devs go with Provider in their state management solution because it does provide this option and doesn't restrict them to best practices.

Are we speaking about the same provider here ? because I've rarely seen someone more obsessed with enforcing his own practices than Remi πŸ˜‚ And he's done a brilliant job doing that and the package benefited from that πŸ™‚ provider indeed offers overloads for this kind of situations. But I would still refactor and aim for having at most 2 BlocBuilder's in a widget simply because those overloads would be way too verbose for my taste. If I'd ever need one I'd take it as a sign that I'm probably off the track.

ThinkDigitalSoftware commented 4 years ago

Well yes, the same provider but then again, the package doesn't aim to solve state management as you know, so it's not concerned with how developers use it outside its scope. I guess that's a good point in favor of keeping it out, and a reason why I'm a die hard fan of the flutter_bloc family.

Since I didn't have authorization to spend the time to refactor this properly, flutter_bloc didn't come through for me with a solution to my problem and using combine latest took me many lines more to implement than having it done for me. We could be a lesser used library for the elites or have wider adoption with inclusion and features. The only issue I see is forcing @felangel to make an example for this in the docs without throwing up! πŸ˜‚

narcodico commented 4 years ago

I guess that's a good point in favor of keeping it out, and a reason why I'm a die hard fan of the flutter_bloc family.

Bad news for you, flutter_bloc uses provider under the hood, you can't escape it πŸ˜‚

The only issue I see is forcing @felangel to make an example for this in the docs without throwing up! πŸ˜‚

I wish you the best of luck 😬

ThinkDigitalSoftware commented 4 years ago

By keeping it out, I was referring to the MultiBlocProvider

felangel commented 4 years ago

Sorry for being late to the party haha.

The only issue I see is forcing @felangel to make an example for this in the docs without throwing up!

@ThinkDigitalSoftware an example of what?

By keeping it out, I was referring to the MultiBlocProvider

So you’re saying you prefer MultiBlocProvider not be part of flutter_bloc?

ThinkDigitalSoftware commented 4 years ago

You usually put good use cases for the widgets you introduce. You're going to have to find one for this, lol

I was saying that it's an argument against including it that sounds reasonable, but there are more on the 'for' side

MichelMelhem commented 4 years ago

For me a common use case is that in the same page I need to get some user data (ex: if the user has a valid account) from like an Authbloc , but in addition of that I have to fetch some other data (ex: a list of create todos). In this case it would be useful to have this bloc. But in fairness i think that this type of cases are pretty mandatory and do not need a special widget.

But what I think would be more useful to still have the classic bloc builder and in addition of that have a second optional argument that give the possibility to pass an optional bloc so it would not break actual code . An example of my thoughts :

Blocbuilder<bloc1, stateofbloc1><bloc2,stateofbloc2>(
context: context,
builder:(context, bloc1, stateofbloc1, bloc2,stateofbloc2)
{return ...}

)

VoilΓ  !

PS: I know this feature request is quite old but as in some niche cases this feature could be useful and I think my solution could make everyone happy without over-ingeniring the lib

ThinkDigitalSoftware commented 4 years ago

I don't believe you can optionally place generics like that. It would have to be a separate class built for handling 4 generic types, which would be what we were discussing

Allan-Nava commented 4 years ago

For me a common use case is that in the same page I need to get some user data (ex: if the user has a valid account) from like an Authbloc , but in addition of that I have to fetch some other data (ex: a list of create todos). In this case it would be useful to have this bloc. But in fairness i think that this type of cases are pretty mandatory and do not need a special widget.

But what I think would be more useful to still have the classic bloc builder and in addition of that have a second optional argument that give the possibility to pass an optional bloc so it would not break actual code . An example of my thoughts :

Blocbuilder<bloc1, stateofbloc1><bloc2,stateofbloc2>(
context: context,
builder:(context, bloc1, stateofbloc1, bloc2,stateofbloc2)
{return ...}

)

VoilΓ  !

PS: I know this feature request is quite old but as in some niche cases this feature could be useful and I think my solution could make everyone happy without over-ingeniring the lib

How can I implement this feature? I need to handle two BLoCs one for LiveEvents and one for Categories

felangel commented 4 years ago

@Allan-Nava as of now you can nest the BlocBuilders.