felangel / bloc

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

[Feature Request] How about a BlocConverter in a kind of StoreConnector in flutter_redux? #390

Closed UnoDeTantos closed 5 years ago

UnoDeTantos commented 5 years ago

Nothing better than an example:

BlocConverter<AppState, bool>(
    bloc: appBloc,
    distinct: true, // True by default, please :)
    converter: (state) => state.favorites.contains(myWhatEver),
    builder: (context, isFavorite) {
        [...]
    }
)

This builds only if myWhatEver is added/removed from favorites.

I know that exists BlocBuilder with condition, but following the example:

BlocBuilder(
    bloc: appBloc,
    condition: (oldState, newState) => oldState.favorites != newState.favorites,
    builder: (context, state) {
        if( state.favorites.contains(myWhatEver) ) {
            [...]
        } else {
            [...]
        }
    }
)

This builds if something is added/removed from favorites. condition could be oldState.favorites.contains != newState.favorites.contains but it's worst in my opinion.

I leave here a possible implementation: (NOT TESTED!!!)

typedef BlocConverterFunction<S,C> = C Function(S state);
typedef BlocConverterBuilder<C> = Widget Function(BuildContext context, C data);

class BlocConverter<S,C> extends StatefulWidget {
  final Bloc<dynamic,S> bloc;
  final bool distinct;
  final BlocConverterFunction<S,C> converter;
  final BlocConverterBuilder<C> builder;

  BlocConverter({
    Key key,
    @required this.bloc,
    @required this.converter,
    @required this.builder,
    this.distinct = true
  }) : assert( bloc!=null ),
       assert( converter!=null ),
       assert( builder!=null ),
       super(key: key);

  @override
  _BlocConverterState createState() => _BlocConverterState<S,C>();
}

class _BlocConverterState<S,C> extends State<BlocConverter<S,C>> {
  C current;
  StreamSubscription<S> _subscription;

  @override
  void initState() {
    _prepare();
    super.initState();
  }

  @override
  void didUpdateWidget(BlocConverter<S,C> oldWidget) {
    _subscription?.cancel();
    _prepare();
    super.didUpdateWidget(oldWidget);
  }

  void _prepare() {
    current = widget.converter(widget.bloc.currentState);
    _subscription = widget.bloc.state.skip(1).listen(_handleData);
  }

  void _handleData(S state) {
    final data = widget.converter(state);
    if( !widget.distinct || data!=current ) {
      setState(() {
        current = data;
      });
    }
  }

  @override
  void dispose() {
    _subscription?.cancel();
    super.dispose();
  }

  @override
  Widget build(BuildContext context) => widget.builder(context, current);
}
felangel commented 5 years ago

Hi @UnoDeTantos 👋 Thanks for the feature request!

Can you please elaborate a bit on what the advantages of BlocConverter would be when compared to the conditional BlocBuilder? I'm not sure I understand why the BlocBuilder condition is the worst approach.

Also, I think this makes sense in redux because you have one global state whereas with bloc your state should be distributed across multiple blocs. I think having such a widget would encourage people to have a single AppBloc that has the entire app state which goes against the bloc paradigm.

Thoughts?

UnoDeTantos commented 5 years ago

Can you please elaborate a bit on what the advantages of BlocConverter would be when compared to the conditional BlocBuilder? I'm not sure I understand why the BlocBuilder condition is the worst approach.

Following the example, with BlocConverter my FavoriteButtonWidget only builds if its status changes. If I use conditional BlocBuilder, it rebuilds every time I add or remove a favorite. I'm really new in flutter and don't know how costly is the build operation but I prefer don't if not necessary.

The oldState.favorites.contains != newState.favorites.contains alternative is worse because makes favorites.contains three times, two to compare and another building (to know its status).

Also, I think this makes sense in redux because you have one global state whereas with bloc your state should be distributed across multiple blocs. I think having such a widget would encourage people to have a single AppBloc that has the entire app state which goes against the bloc paradigm.

And the problem againts an AppBloc is...? :)

I am not saying an entire app in AppBloc, but there are things that should be global and interact as a whole for its complexity. It's the global app status.

mapEventToState is beautiful, is the equivalent to middelware + reducer in Redux. It gives me the flexibility to have an AppBloc and a lot of other blocs (for screens, forms, widgets, etc..) and I really like it.

UnoDeTantos commented 5 years ago

Another approach:

BlocBuilder(
    bloc: appBloc,
    convert: (status) => status.favorites.contains(myWhatEver),
    condition: (old, new) => old!=new,
    build: (context, isFavorite) {

    }
)

convert (or transform) is a simple passthru by default.

I don't like this solution because adds complexity using BlocBuilder, I prefer to separate these functionalities.

felangel commented 5 years ago

Hey @UnoDeTantos, you should be able to use a conditional BlocBuilder to only rebuild if a status changes as well. You just need to return previousState.status != currentState.status in the BlocBuilder condition.

In general, build operations are not that costly (flutter rebuilds for animations on every frame etc...) so I would not worry about over-optimizing your application.

In addition, I agree that there are some things that make sense to be global like Authentication, or Theme related state but I wouldn't recommend combining those into a single bloc. Instead, I'd recommend two blocs: AuthenticationBloc and ThemeBloc so that each has a single responsibility allowing only the parts of the application that care about theme or authentication respectively to update.

Lumping everything into a single AppState quickly becomes complex and inefficient because entire portions of the application will have to rebuild just because an unrelated part of the state changed. With redux, this is unavoidable because you are required to have a single global store. With the bloc library, however, this is something I would really like to avoid and in my opinion having a converter takes us one step closer to redux as opposed to forcing developers to split up their state into small, specialized blocs.

Thoughts?

UnoDeTantos commented 5 years ago

At this point I think I'm missing something :)

Let's see another example, forget all about AppBloc, just a simple shopping cart bloc with this state: (a list of product ID to simplify)

class ShoppingCartBlocState extends Equatable {
    final List<int> cart;
    ShoppingCartBlocState(this.cart) : assert(cart!=null), super(cart);
}

Now, I want to show on every product if it's in the shopping cart (if not, add to cart, etc...):

class InShoppingCart extends StatelessWidget {
    final ProductModel product;
    InShoppingCart(this.product) : assert(product!=null);

    @override
    Widget build(BuildContext context) {
        return BlocConverter<ShoppingCartBlocState, bool>(
            bloc: BlocProvider.of<ShoppingCartBloc>(context);
            distinct: true,
            convert: (state) => state.cart.contains(product.id),
            build: (BuildContext context, bool inCart) {
                return Text(inCart ? "Yes" : "No");
            }
        );
    }
}

Widget for product:5 don't builds if product:6 is added to the shopping cart, only builds if it self is added/removed. BlocConverter saves internally the converted value to compare (bool in this case).

Could you replay this widget with BlocBuilder?

felangel commented 5 years ago

@UnoDeTantos I'm not sure I understand the example.

You could just refactor this to have a bloc for the Products which handles determining whether or not a product is in the cart.

class Products extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return BlocBuilder(
      bloc: BlocProvider.of<ProductsBloc>(context);           
      builder: (BuildContext context, ProductsState state) {
        if (state is Loaded) {
          return Listview.builder(
            itemCount: state.products.length,
            itemBuilder: (BuildContext context, int index) {
              return InShoppingCart(state.products[index]);
            },
          );
        }
      }
    );
  }
}

The ProductsState could look like:

abstract class ProductsState {}

class Loaded extends ProductsState {
  final List<Product> products;
  Loaded(this.products);
}
class Product {
  final int id;
  final bool inCart;

  Product(this.id, this.inCart);
}

This would allow your InShoppingCart Widget to be super simple:

class InShoppingCart extends StatelessWidget {
  final Product product;
  InShoppingCart(this.product);

  @override
  Widget build(BuildContext context) {
    return Text(product.inCart ? "Yes" : "No");
  }
}

In generally, I would really prefer to keep the library as simple as possible because I think a lot of these problems can be solved by having the right app architecture as opposed to having lots of "specialized" widgets that do very similar things.

In addition, do you currently have any performance problems due to widgets rebuilding? If so I would love to take a closer look and help optimize the code but if not I would not try to over-optimize prematurely.

Let me know if that helps and thanks 👍

UnoDeTantos commented 5 years ago

Well, I see your point and understand that you prefer to keep the library as simple as possible, I can accept that :)

What I don't understand is how you remove the ShoppingCartBloc (which handle all the logic for a shopping cart, add, remove, how many, etc), but that is a subject for another conversation.

Many thanks for sharing your time! :)

PS: InShoppingCart was an example to show a point of view, in real world it could be a 'Buy now' button which shows if product is already in the shopping cart, and that button could be used on multiple other widgets because a product could be displayed in different forms. I mean, yes, it's a specialized widget, but a reusable one :)

felangel commented 5 years ago

I’m not saying to remove the ShoppingCartBloc haha sorry if I was unclear. I meant have the ProductsBloc have a dependency on the ShoppingCartBloc. The setup would be very similar to the todos app where the filtered_todos_bloc has a dependency on the todos_bloc.

Let me know if that helps clear things up 👍

UnoDeTantos commented 5 years ago

Opsss... more clear now!!!

ProductsBloc is listening ShoppingCartBloc and updates inCart consequently.

But... it builds ListView every time a product is added to the shopping cart hahaha ... I know, I know, Flutter is optimized for that, only paint what is changed :)

Again, many thanks for your time!

felangel commented 5 years ago

Yup haha and Listview.builder only renders what’s visible so it’s even more optimized 👍

Closing for now but I’m more than happy to continue the conversation if you have additional questions, comments, or concerns.

Thanks!

YeungKC commented 4 years ago

Hi @felangel , I don’t know if I should reply here, but let me talk about it~~ I think it needs a BlocConverter, such as the following example:

    BlocBuilder<TodoBloc, List<Todo>>(
      condition: (previous, current) => previous.length != current.length,
      builder: (context, list) => LengthWidget(list.length),
    );

In this example, we only care about the length for ui, but I need to implement the condition, and at the same time, I also care about the length in the builder, but in fact, I repeated the work.

YeungKC commented 4 years ago

I read your conversation carefully and still think that we need it. I can understand that you want a single function, which will actually improve the quality of the code. I also hope, however, that I think that too many functions will affect the quality of the code.

For example, ShoppingCartBloc's length problem, yes, of course I can increase the length to the state, but, I think it may be easier to use the list directly.

If there is no function similar to BlocConverter, I can also achieve it through streambuilder, but it is a little more complicated...

krevedkokun commented 4 years ago

Right now we can't do something like this

BlocBuilder<CounterBloc, int>(
  bloc: _bloc.where((s) => s > 0),
  builder: (ctx, state) => Text(state.toString()),
)

because BlocBuilder expects subclass of Bloc and stream transformers return Stream. That's pretty basic example, but in more complex cases we can filter bloc by state and then use condition to filter it by state data. We don't really need different widget for that, I think, but we can change signature of BlocBuilder to something like this

class BlocBuilder<B extends Stream<S>, S> extends BlocBuilderBase<B, S> {}