Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Side-effects (events) without overriding state #812

Closed
chriswiesner opened this issue Jan 28, 2020 · 12 comments
Closed

Side-effects (events) without overriding state #812

chriswiesner opened this issue Jan 28, 2020 · 12 comments
Assignees
Labels
question Further information is requested

Comments

@chriswiesner
Copy link

Hi,

I'm having a Bloc for a list with following states:
Empty, Loading, Loaded(List<> data), Error(errormessage)

Adding side-effects with BlocListener has the disadvantage of overriding the actual "data" state.

For example I want to issue side-effects from my Bloc (like Navigating to detail screen, showing snackbar).
This yields a state NavigateToDetail(...) which is overriding the Loaded() state and my list would be empty when I come back from the detail.

For navigation I guess I should use a global Bloc handling the navigation, but the issue still remains for the Snackbar use-case.

Wouldn't it be better to separate the side-effects from the actual (data)state of the Bloc?

I guess I'd need to use a separate Stream for my side-effects. But then listening/disposing in the UI is a bit verbose.

Are there any recommended solutions?

@felangel
Copy link
Owner

Hi @chriswiesner 👋
Thanks for opening an issue!

Several things I would recommend considering are:

  1. Does the navigation depend on business logic? If no, you don't need to introduce navigation based states in your bloc.
  2. I would highly recommend not having anything in your states be UI specific (ie ShowSnackBarState). Instead, I would keep my states generic, describing the feature/use-case (WeatherLoadFailure). I would also generally recommend having separate states (subclasses) when the states are exclusive (I can't have both an error and data simultaneously). If you have cases where both are possible then I would recommend having multiple properties on a single state
class WeatherState {
  final Weather weather;
  final bool hasError;
 
  ...
}

Then you can add a copyWith to your state and yield state.copyWith(hasError: true) for example which would allow you to retain the previous data (if any) and also potentially show an error via BlocListener.

Of course you can always add another stream for side effects but in my opinion that adds a significant amount of additional complexity which you can usually avoid if you structure your states properly.

Hope that helps 👍

@felangel felangel self-assigned this Jan 28, 2020
@felangel felangel added question Further information is requested waiting for response Waiting for follow up labels Jan 28, 2020
@chriswiesner
Copy link
Author

Thanks for the fast response.

  1. yes the navigation step is issued/dependent by/on the logic of the Bloc, therefore I cannot directly issue it from the UI. Would you recommend having a NavigationBloc and call that one from my Bloc?

  2. in my case I have a list and I issue an action on an item in the list from the UI. then the Bloc is applying some actions on the listitem and the UI should then show a Snackbar with an undo action.
    So the list (one item is changing) will be updated with a Loaded(...) state. But additionally I want to show the snackbar as the action was performed, so adding a property assignItemCompleted to the state does not seem right to me.

@felangel
Copy link
Owner

No problem!

I wouldn't recommend a NavigationBloc because again blocs should not have any dependency on Flutter or the UI. I would recommend handling navigation within a BlocListener in cases where you have business logic dependent navigation.

Are you able to share a link to a sample app which illustrates the problems you're facing? It would be much easier for me to give suggestions for a concrete sample app. Thanks!

@chriswiesner
Copy link
Author

There would be no dependency to flutter imho. It's just issuing events for the UI like NavigateTo().

Handling navigation with BlocListener would imply that I have a Loaded(...list, navigateToDetail = true) state, which is not really what I want. I would need to clear the navigation flag once I navigate, otherwise it would again navigate once a come back from the detail.

I'll try to create a sample app.

@felangel
Copy link
Owner

I don't think the extra property should be necessary. Awesome! Don't spend too much time on the sample...just throw something super simple that mimics your current setup and what you're trying to accomplish 👍

@chriswiesner
Copy link
Author

here is the sample repo: https://github.com/chriswiesner/blocSample/blob/master/lib

it's about a listview where you can "assign" items by swiping left/right.

Another similar case for side-effects I came across now was saving an entity. You press a save button. the Bloc saves the entity but I want to listen for the result of the saving process. So if it fails I'll stay on the screen (having the entity still displayed) - but if it succeeds it should close the screen.
having a flag saveResult in the state for the success is confusing. what would be the inital state of the success-flag? (it can't be either true or false when I did not issue the saving process yet)

@felangel
Copy link
Owner

@chriswiesner thanks for putting together the sample! I'll have a look later today and will try to open a pull request with any suggestions 👍

@felangel felangel removed the waiting for response Waiting for follow up label Jan 30, 2020
@felangel
Copy link
Owner

@chriswiesner I opened a pull request with some suggestions. I'm sure I can clean it up even further if I had a better understanding of the requirements but hopefully it gives you a good sense of how to structure things.

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

@chriswiesner
Copy link
Author

Thanks for taking the time and the suggested solution. (I commented my thoughts)

The one issue I'm now having is that the ListItem data is changing when I assign an item. (formerly I updated the whole list, which was anyhow not the best approach).
Now I'd need to hold the itemdata in the ListItemBloc and still emit (maybe even an additional) event to rebuild that item next to the AssignSuccess, right?

@chriswiesner
Copy link
Author

also the items are used in a "grouped" list - where the unassigned items are grouped together in a section and once it is assigned not only the item data is update, also the list of items is changing as it is now in the grouped section. therefore I need to update the list of items if an assignment of an item is successful.

would i listen for the assignSuccess and then issue a refresh on the listBloc?

@felangel
Copy link
Owner

No problem! I would recommend holding the the view model for the ListItem in the ListItemBloc's state.

Now I'd need to hold the itemdata in the ListItemBloc and still emit (maybe even an additional) event to rebuild that item next to the AssignSuccess, right?

Not sure what you mean. You shouldn't need to emit additional events just to rebuild items.

also the items are used in a "grouped" list - where the unassigned items are grouped together in a section and once it is assigned not only the item data is update, also the list of items is changing as it is now in the grouped section. therefore I need to update the list of items if an assignment of an item is successful.
would i listen for the assignSuccess and then issue a refresh on the listBloc?

I would manage the groups via the state of the list bloc. You can add a GroupChanged event to the ListBloc when you assign a ListItem.

Hope that helps 👍

@chriswiesner
Copy link
Author

Thanks, yes for the first point It seemed a bit wrong to me to always pass the data/state (viewmodel) with the event (e.g. ItemAssignSuccess) but ok.

For the second point I ended up as you suggested, to emit a updateItem event to the list bloc.

Thanks again taking the time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants