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

[Discussion] Data flow / event passing between blocs #1072

Closed andreavaccari closed 4 years ago

andreavaccari commented 4 years ago

Hi @felangel, thank you for the excellent library, thorough documentation, and relentless support.

Is your feature request related to a problem?

My team and I are using bloc to manage all our state needs: global and local, persistent and ephemeral. We're struggling to implement requirements that involve complex event passing among blocs. We would appreciate your advice on this, which we believe could help other developers.

In essence, we need to understand what to do when a bloc has some state, derived from a parent bloc, and both blocs can modify the state. In this case, the changes from the parent bloc can overwrite the changes in the child bloc, unless we lift all events and state management to the parent bloc, or we force the parent bloc to push events to the child bloc instead of modifying the state directly.

The former solution is pretty much what redux does, and it partly defeats the point of using bloc. The latter solution is cumbersome, as child blocs would have to "register" themselves with the parent bloc, which would not otherwise be able to find them, and sibling blocs would have to send events up and down the tree via a common ancestor.

Describe the solution you'd like

To illustrate how we've dealt with this so far, I am including a more extensive description below. I tried to strike the right balance between clarity and conciseness. I hope it will be clear enough and not too long. As I believe others will have similar issues, I'll be happy to include the result of this discussion in your documentation.

Overview. Let's say a app is used to manage multiple lists of todos:

Entities. The core domain entities are the following:

Features. Besides the traditional "todo" features, we have these requirements:

Approach. Our approach is inspired by the examples in your docs:

AppBloc

HomeBloc and HomeScreen

TodoListBloc and TodoListScreen

TodoBloc and TodoWidget

Problem. Modifying a Todo from both TodoBloc and TodoListBloc

  1. Let's say a user changes the text of a Todo by typing a character without triggering a significant event

    • Now, the Todo in TodoBloc is different from the respective entity in TodoListBloc and AppBloc
  2. Let's say the user toggles the todo by tapping a button on the ToolbarWidget

    • ToolbarWidget adds an event in TodoListBloc, which updates the Todo entity to mark it completed
    • The new state is propagated from TodoListBloc to TodoBloc, and the changes made in step 1 are lost

As stated at the beginning, the problem is that, when a bloc has some state, derived from a parent bloc, and both blocs can modify the state, an event in the parent bloc can overwrite the changes in the child bloc.

Describe alternatives you've considered

We've identified two possible solutions to this problem:

  1. Lift the state from the child bloc to the parent bloc, and add events to manage the history explicitly.

    • When the user types on the keyboard, TodoBloc immediately adds the TodoTextChanged event in TodoListBloc.
    • When the user reaches a significant point, TodoBloc adds the SaveHistorySnapshot event in TodoListBloc.
  2. Allow the parent bloc to add events in the child bloc, which is the only bloc allowed to modify the state of its Todo

    • The TodoBloc behaves as described above in the approach section.
    • When the user taps on the ToolbarWidget, the latter adds a TodoToggled event to ListBloc, which adds an equivalent event to TodoBloc

Additional context

The issue with the first solution above is that you quickly end up with a "god bloc", where one app-level bloc is responsible for all events. For example, if we lifted the events in TodoBloc to TodoListBloc, by the same reasoning we should lift all events in TodoListBloc to HomeBloc or AppBloc to account for events that move Todos from one list to another.

The issue with the second solution is that TodoListBloc wouldn't know which child TodoBloc to add events to, unless we also implement a system where a bloc register its active state when its related widget is in focus. Plus, a tap on the toolbar would generate an event to the TodoListBloc, only to generate an equivalent event from the TodoListBloc to the TodoBloc.

As of now, we have adopted solution 1. We're managing the issue of maintaing a "god bloc" by using mixins. We have one file per event, where we declare a event class and a mixin with the handler for that event (overridingmapEventToState). We're accepting that we have one large state, sometimes filled with variables that are null because not relevant in the current context of the app.

Is there a better approach? Thank you in advance for your help!

felangel commented 4 years ago

Hi @andreavaccari πŸ‘‹ Thanks for opening an issue and for the positive feedback, I really appreciate it!

Are you able to share the existing app or a sample application which illustrates the problem you're facing? While I really appreciate the detailed description, I feel I would have much more context if I were able to see your current setup. Alternatively, would you be willing to set up some time to go over the current implementation via Live Code Share, Discord, Zoom, etc...?

My initial response without additional context is to try to ensure that you establish a single source of truth for your data. In this case, I would recommend avoiding an AppBloc (we can still achieve offline mode) and having the TodoListBloc be the source of truth of Todos information. I would recommend that all todo related modifications go through the TodoListBloc and that you introduce feature-level blocs which subscribe to the TodoListBloc and expose a "View Model" to the feature UI without allowing direct todo modifications from the feature-level blocs. With this type of setup you can retain a unidirectional flow of data and maintain a single source of truth which should address the problems you're facing.

Just want to emphasize that I'm more than happy to take a look at some code and jump on a call as needed to help your team resolve this problem as quickly as possible πŸ˜„

If you haven't already, consider joining the discord server and sending me a message so we can try to setup some time to do a deeper dive.

Cheers!

andreavaccari commented 4 years ago

Thank you very much for the prompt response! I'll share an example app in the next 24 hours.

In the meantime, I wanted to add that we already attempted to use TodoListBloc as you suggested. However, the requirements 1) to keep all data available offline, 2) to be able to move todos between lists, and 3) to be able to undo a move in the context of the source list history, raised the same problems that I described above.

In other words, we were having problems mutating Todos in different blocs, so we aggregated all operations in TodoListBloc, but then we were having problems mutating TodoLists, so we aggregated all operations in TodoApp. My understanding is that this is exactly the design thinking behind redux and its global store approach.

I'll publish a simple app in a new repo soon. I'll comment here again when it's available.

Thank you! πŸ™

felangel commented 4 years ago

No problem and thanks so much for taking the time to put together a sample app πŸ™

...but then we were having problems mutating TodoLists

I'm not sure I fully understand what you mean by this. Can you elaborate?

andreavaccari commented 4 years ago

Hi @felangel, I prepared a sample todos app based on your flutter_todos example.

The application differences from your original example are:

The implementation differences from your original example are:

What is the problem?

Observation 1:

Observation 2:

The problem:

If a downstream bloc state has a modified Todo, a state change in a upstream bloc will overwrite it. For example, if you change the text of a Todo (handled in TodoBloc), and then you tap the toggle button (handled in FilteredTodosBloc), the modified text is overwritten with the original from the upstream bloc.

My example is contrived by necessity, but illustrates a real problem:

Possible solutions

1. Lifting state

2. Pushing events downstream

3. Pushing events upstream

Our solution

We are using solution 1 at the moment. We have one large bloc that handles everything. We understand this is contrary to your recommendations, but it scales well and is easy to understand. You may have noticed that we structure our files a bit differently around blocs. We use part of directives and mixins to define one event and its handler in one file, making it a lot easier to manage blocs with tens or hundreds of events.

The main downside of this approach is a bloated state class. We have a few ideas on how to improve that but haven't settled on one yet.

Thank you for reading this far. I'd love to hear what you think.

felangel commented 4 years ago

@andreavaccari thanks so much for the really detailed explanations and sample app! I will take a look this weekend and get back to you by Monday πŸ‘

andreavaccari commented 4 years ago

Thank you, @felangel. Please take your time. This is not an urgent matter, but I think it's an important one. Thank you again for your support, it's very inspiring to see you in action. Have a great weekend!

felangel commented 4 years ago

@andreavaccari apologies for the delay...I'm planning to dig into this tomorrow.

andreavaccari commented 4 years ago

Thank you for the update. Please take your time!

felangel commented 4 years ago

@andreavaccari I believe I managed to refactor the existing todos example to meet your specifications. The main modification I made was I ensured that the List<Todo> maintained in the TodosBloc state was the source of truth for the FilteredTodosBloc.

In the refactored version, the FilteredTodosBloc cannot modify the List<Todo>, it can only modify the applied filters. This ensures a unidirectional flow of data which eliminates the state overwrites you were seeing. I wasn't sure if the "manual save" functionality you added was a feature or a workaround so for the current example I made the assumption that all changes should be persisted in real-time. If my assumption was incorrect, I'm happy to modify the example accordingly but I believe the only change would be extracting the call to saveTodos into a TodosSaved event handler rather than saving automatically with each change.

Check it out and let me know if it addresses all of the aforementioned concerns and sorry for the delay!

Edit:

Just re-read your description

It is not desireable to persist every change in TodoBloc as it happens

I'll refactor the example sometime tomorrow to handle maintaining a local copy and only saving when the user explicitly decides to.

Cheers!

felangel commented 4 years ago

@andreavaccari just pushed additional changes to hopefully address all of your concerns. I introduced a TodoBloc to manage the local state of an individual todo and used hydrated_bloc to handle persisting local changes. I then made the TodoBloc subscribe to the TodosBloc and whenever a change concerning the current todo had occurred, I handled merging them (favoring the local changes unless the cache had been invalidated). Check out the complete source code and let me know what you think.

Cheers!

felangel commented 4 years ago

@andreavaccari just checking in to see if you and your team have had a chance to review the solution, thanks!

andreavaccari commented 4 years ago

Hi @felangel, thank you for your patience in waiting for my reply. Your refactor is very much in line with what I have done for my project as well. You also showed me how to use hydrated_bloc, which was something in my todo list (pun intended!). Thank you for that!

For state management, I also ended up with a strategy that keeps a local copy and a "reference" copy in the downstream bloc, and uses a merge function to update the state by comparing the local copy, the reference copy, and the inbound copy. For each property of the state, if the local property is different from the reference property, we keep the local property, else we use the inbound property.

For event passing, I also ended up using events that are sent from the downstream bloc to the upstream one. I tried hard to find a solution that avoided triggering events from one bloc to another, but I concluded it's not a bad idea after all, and it's certainly the cleanest approach for this type of situation.

Given the two points above, the system works well, but only if the UI only uses the state of the downstream bloc. To illustrate this point, I made a tweak to your code, and I'll send a PR after submitting this comment. Please take a look at the commit messages to understand the changes, and let me know how you'd handle the problem I purposely introduced.

Thank you again for all your help!

yun-cheng commented 4 years ago

Hi, @felangel , I have a similar problem regarding complex data flow between blocs.

I'm new to Flutter and Bloc design pattern, but I've read your all documentation and tutorials. Now I'm designing my app, but encounter a data flow problem, that two blocs need to interact with each other.

Using this Todo app for example, suppose we have a new "keywords" tab, and it'll show top 5 keywords that frequently shows in the active Todo list. So we build a new KeyWordsBloc to do this. Right now just let KeyWordsBloc subscribe to TodosBloc can show what we want. However, if we let KeywordItem also be dismissible, and it'll let all todos that include this keyword dismissed.

To do so, my first solution is to let TodosBloc subscribes back to KeyWordsBloc. So when we dismiss a keyword, it'll let all related todos dismissed, and than new top 5 keywords updated. But in a more complex scenario, this solution might cause iterations between two blocs until no state updated.

So my second solution is moving the iterations part into KeyWordsBloc, yielding a state with final keywords and final todos, and adding a TodosEvent with final todos. (Although this might not break the "one bloc dealing one state" rule, but what this solution does is just take the TodosBloc's state out, cooking in the KeyWordsBloc, passing new state into TodosBloc.)

I don't know which approach is better or is there other solution for this problem?

Thanks!

felangel commented 4 years ago

@yun-cheng great question! In this scenario I would be inclined to make the KeyWordsBloc subscribe to the TodosBloc in order to compute the keywords to show but when it comes to dismissing all todos for a particular keyword, I would recommend having that be managed by the TodosBloc. You can add an event like TodosDismissedByKeyword(keyword) which would cause the TodosBloc to wipe out the relevant todos, emit a new state, and the KeyWordsBloc would update its own state as a result. In this setup, the data is always moving from top to bottom.

Let me know if that helps and sorry for the delayed response!

felangel commented 4 years ago

@andreavaccari sorry for the delayed response! I'm planning to take a look at your PR tomorrow πŸ‘

felangel commented 4 years ago

@andreavaccari sorry for the delay! I finally had some time and opened a pull request in which I added a new commit on top on your existing commits to attempt to fix the local completion state issue.

Let me know if that helps and apologies for the delay!

andreavaccari commented 4 years ago

Hi @felangel, thank you for following up and the above. I really appreciate you taking another pass at this. I had to shift my focus on another project, and I need more time to take a look at your changes.

Would it be ok to keep this issue open a little longer until I get back to you?

felangel commented 4 years ago

@andreavaccari of course! πŸ™‚

mzafer commented 4 years ago

Hi @felangel, first thanks for the awesome library !.

Building on the conversation on this thread, how can I handle errors across blocks.

For example if there was an error thrown in _mapTaskUpdatedToState of todos_bloc.dart, how can I catch that error in todo_bloc.dart after I add TaskUpdated in line # 54 (_todosBloc.add(TaskUpdated(state.todo.task)); )

I want to know about that error in todo_bloc so that I handle failures and display error message in the widget corresponding to todo_bloc.

felangel commented 4 years ago

Hi @mzafer πŸ‘‹ I would recommend using try/catch within the blocs and converting the exceptions into error states.

Stream<TodosState> _mapTaskUpdatedToState(
  TaskUpdated event,
  TodosState state,
) async* {
  try {
    await doSomethingRisky();
    yield SomeSuccessState();
  } on Exception catch(_) {
    yield SomeErrorState();
  }
}

Hope that helps πŸ‘

mzafer commented 4 years ago

Thanks, that worked. While I was doing that elsewhere in this case I was trying to listen for onError in todobloc.

todosbloc.listen(
   (state){}, 
   onError:(e) => { 
   //dosomething 
} )

But that did not work, so per your suggestion I instead listen for error state and that works.

todosbloc.listen(
   (state){
    if(state is SomeErrorState){
     //this works.
   } 
  }, 
  onError:(e) => { 
  //dosomething 
  } )
andreavaccari commented 4 years ago

Hi @felangel, I hope you are well. I finally managed to review felangel/bloc_todos/pull/3 and your recommended approach is very similar with what we ended up using for our (more complex) case. I'm going to close this issue, others can reopen it if needed.

Two parting thoughts. First, I think it'd be helpful to have a section in the library docs (perhaps after "Architecture" or as part of "Recipes) where this type of questions is discussed. I suspect there are others out there that are confused about how to manage event passing between blocs.

Finally, thank you again for your help and your work. Your commitment to the community is inspiring!