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

[Question] What is the best practices to work with TextEditingControllers. #2293

Closed ErkinKurt closed 3 years ago

ErkinKurt commented 3 years ago

Hello, first of all thanks for all the hard work, bloc has been an awesome journey for me. :)

Me and my teammates use Bloc pattern in production scale, so we want to follow best practices as much as possible. Lately we were discussing where to put TextEditingControllers. I've checked stackoverflow and it looked like a personal aspect, so I wanted to learn what's the pros/cons or right way to do it.

I know that the cubit/bloc is aimed to separate UI logic into a black box where you put your event and receive a state and react to it. However in our case, our cubit is likely to couple with the screen(UI widget) itself, because; we have a bloc for every individual screen. I was wondering that can I declare TextEditingControllers as bloc members and expose them to screen since the blocs are implemented for specific screens. Since screens are composed with child widgets, instantiating TextControllers in parent and pass them to child widgets was an effort compare to reading the bloc from context and access bloc's text controller.

Can you see any drawbacks in the future that we should be aware of? Is this a good practice to instantiate text controller and listen "inside" the cubit? One problem that I can see (not sure tho) when I throw an event I change the textfield value instead of reacting the state (Which we can consider as a side effect right?). I'll try to illustrate my idea by passing this code snippet. If it's not enough please let mo know and and I can provide an example repo.

class CreatePurchaseBillCubit extends Cubit<CreatePurchaseBillState> {
  PurchaseBillRepository purchaseBillRepository;
  TextEditingController amountTextController;
  TextEditingController vatRateTextController;
  TextEditingController dateTextController;
  TextEditingController purchaseDateTextController;

  CreatePurchaseBillCubit({@required this.purchaseBillRepository})
      : assert(purchaseBillRepository != null),
        super(CreatePurchaseBillState.initial()) {
    purchaseDateTextController = TextEditingController();
    dateTextController = TextEditingController();
    amountTextController = TextEditingController();
    vatRateTextController = TextEditingController();
    amountTextController.addListener(() {
      changeAmount(amountTextController.text);
    });
   }

   void changeDateTime(DateTime dateTime) {
    // Here is the smell I believe. I set the text of the controller from event not from state change 
    dateTextController.text =
        DateFormatter().getDateRepresentation(of: dateTime);
    emit(state.copyWith(date: dateTime));
  }

   void changeAmount(String val) {
      //business logic to calculate amount by checking current state.
      final result = calculateVal(val); 
      emit(state.copyWith(amount: result));
   }
}
ErkinKurt commented 3 years ago

Here is one drawback that I found. I don't know if it's an issue with bloc test, here is a repo that I can share with you. Please run bill_cubit_test.dart and you will see the error.

A TextEditingController was used after being disposed. Once you have called dispose() on a TextEditingController, it can no longer be used.

I know that verify is called after expect and before expect cubit under test closes it's stream. I think this is why I'm receiving this error. I'm commenting this, instead of creating a new issue since I'm not sure it's the right pattern to declare TextEditingController inside of the cubit/bloc or not.

Gene-Dana commented 3 years ago

Hi there @ErkinK ! So the first thing that comes to my mind is that controllers are presentation logic, meaning it's recommended to keep all of that out of your bloc/cubit.

Instead, it's recommended that you use cubit/bloc to update the state model that the presentation layer subscribes to. This means that you should call an event/method on the cubit in order to update a piece of state !

Check out this example from Flow_Builder:

https://github.com/felangel/flow_builder/blob/master/example/lib/profile_flow/profile_flow.dart

In this example, this is the state object that is being updated

class Profile extends Equatable {
  const Profile({this.name, this.age, this.weight});

  final String? name;
  final int? age;
  final int? weight;

  Profile copyWith({String? name, int? age, int? weight}) {
    return Profile(
      name: name ?? this.name,
      age: age ?? this.age,
      weight: weight ?? this.weight,
    );
  }

  @override
  List<Object?> get props => [name, age, weight];
}

And you can see how it is updated here !

class _ProfileAgeFormState extends State<ProfileAgeForm> {
  int? _age;

  void _continuePressed() {
    context.flow<Profile>().update((profile) => profile.copyWith(age: _age));
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(title: const Text('Age')),
      body: Center(
        child: Padding(
          padding: const EdgeInsets.all(8.0),
          child: Column(
            children: <Widget>[
              TextField(
                onChanged: (value) => setState(() => _age = int.parse(value)),
                decoration: const InputDecoration(
                  labelText: 'Age',
                  hintText: '42',

To me, this example is very clear, and even though you are not using FlowBuilder, instead of this context.flow<Profile>().update((profile) => profile.copyWith(age: _age)); you can make it a method from your cubit

felangel commented 3 years ago

Hi @ErkinKurt šŸ‘‹ Thanks for opening an issue!

I would highly recommend against maintaining TextEditingController as part of the bloc. Blocs should ideally be platform-agnostic and have no dependency on Flutter. If you need to use TextEditingControllers I would recommend creating a StatefulWidget and maintaining them as part of the State class. Then you can interface with the control in response to state changes via BlocListener. If you don't need TextEditingControllers then the solution provided by @Gene-Dana using onChanged with TextField works well and eliminates the need to have a StatefulWidget.

Hope that helps!

Closing for now but feel free to comment with any additional questions and I'm happy to continue the conversation šŸ‘

EmirBostanci commented 3 years ago

Hello @felangel , can you please provide an example to manage controllers in the page.

As i understand, i should call cubit functions for text changes in textController listener on the widget instead of cubit, and then respond or apply side effects inside cubit. Do i understand correctly?

jtkeyva commented 2 years ago

Hello @felangel any chance you can provide a sample? I'm trying to figure out how to implement with a Camera Controller. Thanks

felangel commented 2 years ago

Hello @felangel any chance you can provide a sample? I'm trying to figure out how to implement with a Camera Controller. Thanks

You can check out https://github.com/flutter/photobooth/blob/e23be72f669b2581490202169cc1c2b157e2c165/lib/photobooth/view/photobooth_page.dart#L48.

jtkeyva commented 2 years ago

Hello @felangel any chance you can provide a sample? I'm trying to figure out how to implement with a Camera Controller. Thanks

You can check out https://github.com/flutter/photobooth/blob/e23be72f669b2581490202169cc1c2b157e2c165/lib/photobooth/view/photobooth_page.dart#L48.

Thank you! I was under the impression the goal of bloc/cubit was to eliminate stateful widgets entirely. However, I see that is not the case. Any best practices or "make it a bloc" or "make it a cubit" vs just stateful?

CBeening commented 3 months ago

Hello @felangel any chance you can provide a sample? I'm trying to figure out how to implement with a Camera Controller. Thanks

You can check out https://github.com/flutter/photobooth/blob/e23be72f669b2581490202169cc1c2b157e2c165/lib/photobooth/view/photobooth_page.dart#L48.

Thank you! I was under the impression the goal of bloc/cubit was to eliminate stateful widgets entirely. However, I see that is not the case. Any best practices or "make it a bloc" or "make it a cubit" vs just stateful?

This is what confuses me aswell. If I have to use stateful widgets anyway, I might aswell use setstate to manage the state, at least on a smaller scale. So I'm still not sure what to do here as a best practise. @felangel Maybe you could elaborate on this again? Aren't controller usually part of the business logic anyway, so you can manipulate the widget from outside?

premiumbiscuit commented 3 months ago

Hi @ErkinKurt šŸ‘‹ Thanks for opening an issue!

I would highly recommend against maintaining TextEditingController as part of the bloc. Blocs should ideally be platform-agnostic and have no dependency on Flutter. If you need to use TextEditingControllers I would recommend creating a StatefulWidget and maintaining them as part of the State class. Then you can interface with the control in response to state changes via BlocListener. If you don't need TextEditingControllers then the solution provided by @Gene-Dana using onChanged with TextField works well and eliminates the need to have a StatefulWidget.

Hope that helps!

Closing for now but feel free to comment with any additional questions and I'm happy to continue the conversation šŸ‘

Regarding the reasoning behind this comment, it is true that most Blocs should be platform-agnostic to encapsulate business logic. However, in my experience, there is a secondary use for Blocs. Sometimes UI logic becomes really complex (e.g., synchronized animations across multiple widgets, very complex UI configurations based on user behavior), making state management essential to separate the UI logic from the basic usage of that logic.

Before Blocs I would use a combination of a Stateful widget and an InheritedWidget to be able to hold that central UI logic, but since I see that as synonymous to a Bloc I really see no issue in having Blocs/Cubits that specialise in UI logic.