[Question] Issue with multiple tabs, the same api and different filters

vadrian89 commented 2 years ago

Hello Felix, Sorry for bothering you with a question using the issues system, but this is bugging me for a while and I am not sure how to deal with it.

For example, I am working on an app which has multiple tabs, which call the same api with different filters.

To avoid building 6 Cubits just for just for a filter change, I've decided to use 1 cubit for all lists. The problem is when they emit, sometimes, they emit at the sametime and part of the data doesn't make it to the presentation layer.

This is how I call the methods in question:

Future<void> getAllWorkOrders() async {
    await listOpen();
    await listUnread();
    await listInProgress();
    await listOnHold();
    await listCompleted();
    await listAll();
  }

A workaround to avoid them overlapping I do the following:

Future<void> getAllWorkOrders() async {
    await listOpen();
    await repoDelay();
    await listUnread();
    await repoDelay();
    await listInProgress();
    await repoDelay();
    await listOnHold();
    await repoDelay();
    await listCompleted();
    await repoDelay();
    await listAll();
  }

repoDelay() returns a Future.delayed(const Duration(milliseconds: 300)) so I give them time to process. So I am not sure:

is this the proper way to handle doing this sort of work?
is there another way?
is this a bug?
other?

marcossevilla commented 2 years ago

hi @vadrian89! you can also use Future.wait to await all your futures at once.

Let me know if that helps. 👍

vadrian89 commented 2 years ago

@marcossevilla , thanks for you answer.

Honestly I can't because I cannot keep all the tabs blocked in loading until all of them finish. I need to process them as quickly I can, as they come, but sometimes 2 or more come at the sametime.

felangel commented 2 years ago

Hi @vadrian89 👋 Thanks for opening an issue!

Can you please provide a minimal reproduction sample that illustrates what you're trying to achieve and the issue you're running into? Thanks!

vadrian89 commented 2 years ago

Hi @vadrian89 wave Thanks for opening an issue!

Can you please provide a minimal reproduction sample that illustrates what you're trying to achieve and the issue you're running into? Thanks!

Will try my best to make one.

vadrian89 commented 2 years ago

cubit_test.zip

@felangel Here is an example reproducing the issue.

If I add delay, all the colors are changed. If not, only the last is changed, it seems the BlocBuilder is skipping the previous states.

Gene-Dana commented 2 years ago

cubit_test.zip

The best way to send this is through a github repo ! Do you have one already for this project?

vadrian89 commented 2 years ago

@Gene-Dana Sorry for the late response, had some personal problems. I've made a github repository for the example: https://github.com/vadrian89/cubit_test

chonghorizons commented 2 years ago

@Gene-Dana Sorry for the late response, had some personal problems. I've made a github repository for the example: https://github.com/vadrian89/cubit_test

A few things:

Don't use your builder to update state I think the issue is that you're using your builder to change state:
```
    builder: (context, state) {
      state.maybeWhen(
        orElse: () => null,
        color1: (color) => _color1 = color,
        color2: (color) => _color2 = color,
        color3: (color) => _color3 = color,
      );
```
The builder can run once or multiple times or not at all (if things are in the background). And can create weird race conditions, like what you're trying to deal with with your 300 millisecond delay.

Use a respository to coordinate API calls. I think you might want an architecture with one repository that connects to your API and each of your features go through the repository. The repository can handle the waiting.

In two sentences:

If all API calls go through the repository, the repository can coordinate.
If individual widgets have API calls, the individual widgets will have a hard time coordinate.

This is demonstrated in the v8 todos tutorial (not on website yet, under review).But the architecture idea is here: https://github.com/felangel/bloc/pull/2859#issuecomment-987233491

Use streams A lot of thinking seems to be the "fetch" model. Fetch is fine for loading (updating) just that widget when loaded. But it doesn't work well if you have to tell other widgets to change state and re-render.

Use streams, because that's where they excel.

See my writeup in https://github.com/chonghorizons/bloc/blob/chong-docs-todos/docs/fluttertodostutorial.md#stream-vs-fetch

Your underlying API/data provider might not have streams. That's okay. The repository layer can generate the stream from the data provider. In fact, that's what's done in the v8 tutorial.

vadrian89 commented 2 years ago

@chonghorizons thank you for your input, I understand what you mean about using streams but I don't understand the reasoning behind why I shouldn't use builder to set state. Can you elaborate?

I mean, I don't have any problems setting state class member properties when the correct state is emitted.

Also, before using the bloc package I made my own blocs and used Streams + StreamBuilders to achieve the BloC pattern so I assumed all states are sent through a stream which in turn should result in sequential rebuild of the BlocBuilder.

chonghorizons commented 2 years ago

@vadrian89 Thanks for clarifying things, and for the the thanks. WE're all just learning here.

The argument for not Setting state in the builder

It might depend on your use case. In some cases it is definitely not okay. In other cases it's fine. But I think you can always move that set state somewhere else and it's safer. Defensive programming.

Definitely not okay

If your set state has a side effect that you don't want to run multiple times. Not that you are doing this, but if you have an in-game/banking transaction that you want to run exactly once. If the widget rebuilds twice, you have the transaction happen twice.
You might run into an infinite set state loop. You set the state once in your StatefulWidget. This triggers a BlocListener descendant. That might do something that triggers a redraw, and round and round it goes.
Redrawing a parent, which resets the StatefulWidget. If you think about the event flow see previous message, you might accidentally erase your state via some loopy event flow. Your builder does some state setting, a descendent talks to a bloc, then that talks to a parent, and that generates a rebuild. This might be because you have a textController.

Probably okay

navigation. If you navigate to a screen twice, no big deal.
idempotent. This is a fancy word for the idea that running 2 times or 200 times is the same as once. Navigation fits here.
Nothing redraws upstream. If nothing happens upstream, there's no infinite loop.

Defensive way to avoid it.

Simplest: Move your state logic out of builder and into a BuildListener or BuildConsumer. (https://bloclibrary.dev/#/flutterbloccoreconcepts?id=blocconsumer). You copy-paste the setState here. Probably works 80% of the time. Advantage: The BlocConsumer.listener() will (should) run exactly once. build could run multiple times.
Move the setState logic upstream into the Bloc. The state stays in the StatefulWidget. The Bloc does the logic.
Best: Get rid of all your StatefulWidgets and replace with bloc/cubit. I did a cursory check, and the tutorials avoid Stateful widgets entirely. Zero (I didn't search exhaustively). No more TextControllers. The bloc can handle all of this (like in the v8 flutter_todos).
- A big advantage is that this is easier to debug and to test. You can widget Test that the emit(states) are correct. And then, stateless widgets are UI=f(BlocStates), so you can test that.

Highly recommend

Use blocObserver and maybe also log your widget rebuilds (if you are set-stating in them).

tags

Tagging as #bestPractices so I can find these writeups later, possibly #1819.

chonghorizons commented 2 years ago

Also, for minimum code example, I really prefer if you use dartpad. See #3040 for an example of moving your code to a gist that works in dartpad so we don't have to download and run your code.

As an exercise, you might try to modify the dartpad code sample to generate an infinite redraw loop.

vadrian89 commented 2 years ago

Thanks for your elaboration, the thing is I do not use setState inside my builder. I am very aware of the side-effects of calling setState inside BlocBuilder but it's great that you wrote this, maybe someone will bump into my issue, read the comments and see their mistakes.

I don't agree, at the moment, with the fact that I should use only StatelessWidget and let bloc handle everything. I might change my mind on it in the future, but at the moment I want to avoid coupling the presentation layer with the application layer too tightly.

The thing is if I would let bloc handle everything I might end up in a situation where:

a breaking change might come in either Flutter or bloc package which could screw the entire app;
bloc package might get discontinued and the work to replace it would be enormous;

I am aware of the benefits of using StatelessWidget vs StatefulWidget so I understand the reasoning of using BlocBuilder/Consumer with StatelessWidget.

chonghorizons commented 2 years ago

I've added 3 notes in the code below. See the // notes.

You have a //1 StatefulWidget, and then a //2 BlocProvider of a Cubit within it. And then //3 something that directly changes the state variables in the StatefulWidget.

When you directly change the state variables (without using setState), it won't trigger a re-render. See StatefulWidget

It might also help to look at how Widgets are marked dirty.

I'm not 100% sure this is what you're getting at. But, I recommend you put some print statements in the Cubit and the state.maybeWhen and the builder to understand the timing issue.

In this case, it isn't an async issue. And it is not a bug.

class _MyHomePageState extends State<MyHomePage> {
// 1. You have state here.
  Color _color1 = Colors.blue;
  Color _color2 = Colors.red;
  Color _color3 = Colors.green;

  @override
  Widget build(BuildContext context) {
    return BlocProvider(
      create: (context) => ColorChangerCubit()..changeColors(),  // 2. You have state here.
      child: BlocBuilder<ColorChangerCubit, ColorChangerState>(
        builder: (context, state) {
          state.maybeWhen(
            orElse: () => null,
            color1: (color) => _color1 = color,  // 3. You modify the widget state here. It won't trigger a rebuild.
            color2: (color) => _color2 = color,
            color3: (color) => _color3 = color,
          );

chonghorizons commented 2 years ago

If it's an issue that you have a lot of legacy code you don't want to convert from StatefulWidgets to Bloc, that's fair.

The best solution is don't use Bloc. Don't mix.

To mix and shoehorn Bloc into existing, you have to do all this:

make a method in _MyHomePageState that does a setState() to the state. This is the only way to trigger a rebuild of the StatefulWidget. Something like changeColorsInStateful
In blocProvider( create: ... ) attach that method to the Cubit. NOTE: create runs once and runs lazily.
In your mock, add a button that triggers ColorChangerCubit.changeColors()
Within ColorChangerCubit.changeColors(), run the method that triggers a setState and marks the StatefulWidget as dirty.
Debugging: Put a bunch of print statements to make sure the order of things works right.

NOTE: Each time the StatefulWidget re-renders/re-builds, it may make a new BlocProvider. That's because BlocProvider is a descendant. In your minimum example, it doesn't matter, but in other cases it might. You might be able to mess with GlobalKeys to avoid rebuilds, but I'm not sure if BlocProvider exposes GlobalKeys.

You might have an unintended infinite loop. The StatefulWidget rebuilds -> makes a newBlocProvider -> runs setState -> StatefulWidget rebuild ->.... (infinite loop)

There's no easy to break the "descendents are re-rendered" rule for Widgets. You could move BlocProvider above Stateful, but then every re-build of the BlocBuilder will make a new Stateful, unless you use a GlobalKey.

These are exactly the annoying life-cycle things Bloc is built to avoid if you work with the framework.

Tag as #BewareBlocProvidersInsideAnotherBlocBuilderRebuilds #Gotchas

tag

felangel commented 2 years ago

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

felangel / bloc