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] Trying to understand new Todos example #3050

Closed jrmallorca closed 2 years ago

jrmallorca commented 2 years ago

I've been following ResoCoder's Flutter Clean Architecture TDD https://resocoder.com/flutter-clean-architecture-tdd/ but I'm currently struggling to understand how to combine this with the new bloc versions, more specifically with the Firebase Todos example as that is the one most applicable to my project.

I've used the old Todos example https://bloclibrary.dev/#/fluttertodostutorial?id=key-topics which was mainly sequential and I can determine which state to yield. By this, usually it was whether I should say we go to a success or failure state, depending on whether there were any errors when calling a method within the repository. For example, if there were no errors in adding a Todo in the TodosRepository, yield a success state, else yield a failure state.

With the new Firebase Todos example https://bloclibrary.dev/#/flutterfirestoretodostutorial, I believe that _onLoadTodos in the TodosBloc is the one mainly emitting states. Methods such as _onAddTodo will indirectly emit a new state due to the StreamSubscription within _onLoadTodos.

Due to this, using _onAddTodo as an example, would an error in adding a Todo within the TodosRepository be dealt within _onLoadTodos through onError?

jrmallorca commented 2 years ago

I believe my problem stems from not knowing how to properly implement error handling. Would there be any resources on that?

HadiHassan22 commented 2 years ago

Hey @jrmallorca,

So if pre version 8.0.0 you used to do the following:

Stream<TodoState> mapEventToState(TodoEvent event) async* {
    if (event is LoadTodos) {
      yield TodoState.loading();
      try {
        List<Todo> todos = await _repository.fetchTodos();
        yield TodoState.success(todos);
      } catch (e) {
        yield TodoState.failure(e);
      }
    }
  }

in version 8.0.0 you can do the following:

TodosBloc() : super(0) {
    on<TodoEvent>(_onEvent);
  }

void _onEvent(TodoEvent event, Emitter<TodoState> emit) async {
    await event.when(
    loadTodos: () async {
    emit(TodosState.loading());
      try {
        List<Todo> todos =
            await _repository.fetchTodos();
        return emit(TodosState.success(todos));
      } catch (e) {
        return emit(TodosState.failure(e);
      }
    }
);

Hope this helped you, however as for handling errors through onError I would not be able to tell you. I tried looking a bit but I didn't seem to understand if the onError API is meant to replace the try catch on event handling or if it is only for handling only internal errors.

chonghorizons commented 2 years ago

With the new Firebase Todos example https://bloclibrary.dev/#/flutterfirestoretodostutorial, I believe that _onLoadTodos in the TodosBloc is the one mainly emitting states. Methods such as _onAddTodo will indirectly emit a new state due to the StreamSubscription within _onLoadTodos.

Yeah, I believe that is the new pattern being used.

The tutorial for the new v8 tutorial is in review (I just finished the first draft). You'd be a good test case for whether the new docs make sense. The diagram referenced in the "https://github.com/felangel/bloc/pull/2859#issuecomment-987233491 directly addressed the concept you mention: "will indirectly emit a new state due to the StreamSubscription within _onLoadTodos." (the names are different, but the event flow is the same)

Please do take a look at the new tutorial draft I submitted here: https://github.com/felangel/bloc/blob/42901785deadcc73076ffc82595b825edd205a5c/docs/fluttertodostutorial.md#stream-vs-fetch (NOTE: The automatic includes don't load if you view the file on github. If you want it to look "pretty", that takes about 30-60 minutes of setup, described here: https://github.com/felangel/bloc/blob/docs/flutter-todos-v8.0.0/CONTRIBUTING.md#contributing-to-documentation)

PARANOID NOTE: Master branch still refers to the old todos example. The v8 source code is https://github.com/felangel/bloc/tree/docs/flutter-todos-v8.0.0 . Do you know how to clone and run the new branch?

chonghorizons commented 2 years ago

Due to this, using _onAddTodo as an example, would an error in adding a Todo within the TodosRepository be dealt within _onLoadTodos through onError?

If _onAddTodo fails it would trigger onError. You could overwrite onError. But I think in the Bloc, you might have a try catch statement around the _onAddTodo. And then cleanly deal with the exception, like emitting a failed status which the UI can deal with.

I mostly am thinking through the v8.0.0 example that has a status field in all the BlocStates to deal with just these errors. The old todos didn't have a way to communicate an error (like an async, no-network error) to the UI via the bloc state.

The tutorial doesn't have an example on an error on adding a Todo (called EditTodo in v8). But it does have a BlocListener to handle if TodosOverviewBloc gives a failure status.

Short version: Look at the v8 and see it that clears things up. And, if it's cleared up, please close the issue.

jrmallorca commented 2 years ago

Thank you @HadiHassan22 and @chonghorizons for your help. This is really helping me understand about how the new version works now and helps me adapt my program to the new version.

@chonghorizons with regards to the tutorial, it is very understandable. I've actually been following #2859 a bit before this and I am glad to see a draft. There's a few minor grammar problems I believe but it's a draft anyway. Admittedly, I mainly looked at the TodosRepository, TodosOverviewBloc, and EditTodoBloc as those were the key things I wanted to understand and haven't ran the code.

I do have a few questions regarding the example though, rather than the documentation itself. All State classes only include one state with a bunch of fields to indicate the status. Would this be best practice as compared to separate states, or is it just because this is a small app? For example, instead of TodosLoadSuccessState and TodosLoadFailedState, we would have TodosState.success and TodosState.failed?

The old Todos example separated the filtering through another Bloc class. This one doesn't. Would it be preferable to follow this version instead, as the previous version relied on the main TodosBloc and a subscription?

Also, the method _onTodoSaved() in TodosOverviewBloc doesn't look like it's been used as a Todo is being saved through the _onSubmitted() method in EditTodoBloc (from what I can see). Therefore, _onTodoSaved() is effectively redundant in this example and could be removed right?

chonghorizons commented 2 years ago

@chonghorizons with regards to the tutorial, it is very understandable. I've actually been following #2859 a bit before this and I am glad to see a draft. There's a few minor grammar problems I believe but it's a draft anyway. Admittedly, I mainly looked at the TodosRepository, TodosOverviewBloc, and EditTodoBloc as those were the key things I wanted to understand and haven't ran the code.

Thanks for taking a look and the feedback!

I do have a few questions regarding the example though, rather than the documentation itself. All State classes only include one state with a bunch of fields to indicate the status. Would this be best practice as compared to separate states, or is it just because this is a small app? For example, instead of TodosLoadSuccessState and TodosLoadFailedState, we would have TodosState.success and TodosState.failed?

Either way works. They are equivalent to the program.

To nitpick

code-wise, you can't use copyWith if you move between state instances that are separate classes (the old way). At least not in a straightforward way.

In the old Todos, the state only held the list of Todos for the Success status. Other states only have status. To use the old pattern (separate classes) in the new Todo, you could have the state hold the list & the viewOption and the lastDeletedTodo on a Success status. But then, if you pass through a state that isn't Success, you would lose the other fields.

The old Todos example separated the filtering through another Bloc class. This one doesn't. Would it be preferable to follow this version instead, as the previous version relied on the main TodosBloc and a subscription?

There are pros and cons of each way. You could split the TodosOverview to two blocs: TodosList and TodosView. Best way to learn is to try to split it and see.

Also, the method _onTodoSaved() in TodosOverviewBloc doesn't look like it's been used as a Todo is being saved through the _onSubmitted() method in EditTodoBloc (from what I can see). Therefore, _onTodoSaved() is effectively redundant in this example and could be removed right?

I think you are correct. But, more importantly, try it out. You can either: 1) Use find in all files in your IDE to see if the event TodosOverviewTodoSaved ever is sent. ("add") 2) Remove the event and listener and see if your linter complains.

jrmallorca commented 2 years ago

Thank you for all the help. I think this would be enough for me to progress now so I'll close this issue and reopen (another) if necessary.

jrmallorca commented 2 years ago

@chonghorizons sorry, another question. In TodosOverviewBloc, the method _onTodoDeleted() also emits a state while deleting a Todo from the TodosRepository. How will this interact with the emit in _onSubscriptionRequested()? Are there 2 states being emitted?

Edit: From reading the tutorial again, my understanding is that it will emit a state just to save it into the lastDeletedTodo field. Later on, the _onSubscriptionRequested will notice a change within the list of todos and emit a new state accordingly, with the inclusion of the lastDeletedTodo from the previous emit.

If I'm correct then my question would be if order matters? i.e. should we emit state first then delete? or can we also delete then emit state?

Another edit: I think I understand the process from looking at the test cases for the BLoC. It might be helpful to include those in your tutorial perhaps?

chonghorizons commented 2 years ago

@chonghorizons sorry, another question. In TodosOverviewBloc, the method _onTodoDeleted() also emits a state while deleting a Todo from the TodosRepository. How will this interact with the emit in _onSubscriptionRequested()? Are there 2 states being emitted?

Yes, two states are being emitted. and... (below)

Edit: From reading the tutorial again, my understanding is that it will emit a state just to save it into the lastDeletedTodo field. Later on, the _onSubscriptionRequested will notice a change within the list of todos and emit a new state accordingly, with the inclusion of the lastDeletedTodo from the previous emit.

If I'm correct then my question would be if order matters? i.e. should we emit state first then delete? or can we also delete then emit state?

I want to nudge you to try it yourself. Don't be afraid to break the code in learning . Just try it and see. Keep watch of the blocObserver log messages.

Another edit: I think I understand the process from looking at the test cases for the BLoC. It might be helpful to include those in your tutorial perhaps?

I agree. But the tutorial is already long.

Maybe do a short writeup like a "Learner Comment". Use this template.


Recommendation: Read the test files to better understand the bloc Initial confusion: xxx (like the delete firing twice)

What you did: <read docs, specifically xyz>

Solution to the confusion: Instead of XXX, I realized YYY because of ZZZ.

I would support adding more of these "aha" moments somewhere in the docs. Maybe embedded. Maybe a link elsewhere.


It's nearly impossible for me to write an "Aha" since I can't make up confusions. And pretending to be stuck is a poor substitute for actual learners being stuck and overcoming. Can you tell I used to be a teacher?

@jrmallorca

jrmallorca commented 2 years ago

Thank you for all the help. I believe I understand how it works now. I do have another question but I think that would be more appropriate in another issue.