Closed nivisi closed 2 years ago
bartekpacia
commented
2 years ago @nivisi We encountered a similar/almost the same (I haven't read your whole post) problem at LeanCode and created a package to solve it :) https://pub.dev/packages/bloc_presentation
nivisi
commented
2 years ago @bartekpacia thank you, it actually looks good. Gonna try it 😀
nivisi
commented
2 years ago @felangel any chance to have a look at this one?
felangel
commented
2 years ago Hi @nivisi 👋 Thanks for opening an issue and sorry for the delayed response!
A bloc/cubit manage a single piece of state which can change over time. The shape of that state and how the UI reacts to changes in that state are totally up to you as a developer. In the case you've described, I'd highly recommend breaking your bloc/cubit into two (one to manage the state of the photo selection and one to manage the state of the photo upload):
enum PhotoSelectionStatus {
photoSelectionIdle,
photoSelectionInProgress,
photoSelectionSuccess,
photoSelectionFailure,
}
class PhotoSelectionState {
const PhotoSelectionState._({
this.status = PhotoSelectionStatus.photoSelectionIdle,
this.selectedPhoto,
this.error,
});
const PhotoSelectionState.initial() : this._();
const PhotoSelectionState.inProgress() : this._(status: PhotoSelectionStatus.photoSelectionInProgress);
const PhotoSelectionState.photoSelectionSuccess(Photo photo) : this._(status: PhotoSelectionStatus.photoSelectionSuccess, photo: photo);
const PhotoSelectionState.photoSelectionFailure(Object error) : this._(status: PhotoSelectionStatus.photoSelectionFailure, error: error);
final PhotoSelectionStatus status;
final Photo? selectedPhoto;
final Object error;
}And a similar state for the PhotoUploadState.
If the feature involves performing a side-effect you can use a listener and if the feature involves re-rendering you can use a builder.
The important point in my opinion is that the bloc shouldn't make assumptions about how the presentation layer will react to the state change. If we differentiated between "side-effects" and "states" at the bloc level it would imply that the bloc has an opinion about how it is being consumed.
For example, maybe initially you only show a SnackBar if the photo upload fails but later you decide you actually want to render a static error widget on the screen when a failure occurs. Ideally, you shouldn't need to make changes to the bloc to support that change in requirements since it's purely a change in the presentation layer. Alternatively, if we supported two different APIs then a change in the presentation layer would require a change in the business logic component.
Let me know what you think and apologies for the slow response, cheers!
nivisi
commented
2 years ago @felangel your explanation makes it clear and I agree. We don't need to change our blocs if we need to apply changes to the UI layer.
Plus, I had a chance to work with Bloc more closely during last week. Now I think I'm not confused with "why my data should be nullable in the Selected state" :)
So, in general, using a custom state to listen for it and navigate if it's emitted — is a good standard right? I kind of think it is redundant, but shouldn't we put effort in preventing rebuilding the UI when this state is emitted? Like providing buildWhen: state is! NavigationState?
and apologies for the slow response
That's okay! Thank you very much for the answer and your time 🙂 Have a nice day :)
ptrbrynt
commented
2 years ago I raised the issue of side-effects in Blocs recently as I agree it could be a valuable change.
As @nivisi quite rightly points out, it isn't really correct to use a Bloc's state to represent a side-effect. According to the Bloc library documentation:
States are the output of a Cubit [or Bloc] and represent a part of your application's state. UI components can be notified of states and redraw portions of themselves based on the current state.
This means that using a State to represent a side-effect goes against the concept of what a State should be: that is, a representation of part of your application's state. "The application should show a SnackBar now" is not really part of the application's state; it's an event that takes place within the application.
If we were being strictly correct, BlocListener in its current form would not exist as it goes against the fundamental design of the Bloc pattern. Of course, pragmatically it's a useful thing to have as part of the library and I wouldn't take it away, but the point still stands that it breaks the definition above.
If the Bloc library had explicit support for side-effects, we could maintain the strict definition for a State while keeping the useful functionality that BlocListener provides. I believe this would make the Bloc pattern and library easier to learn both conceptually and practically. Forcing developers to think about what are states and what are side-effects will ultimately lead to better code.
I have long felt that using a single State class to also represent both actual UI states as well as side-effects is a bit clunky, and leads to some strange requirements in terms of the code being written. For example, if I have any "side-effects as states" in my Bloc, I have to write a buildWhen check to ensure the BlocBuilder is only building the widget when my state is actually a state, and a listenWhen check to ensure the BlocListener only listens to states which are side-effects.
It would make much more sense, and cause a lot less confusion for developers, for BlocBuilder and BlocListener to have different things to listen to.
I don't entirely agree that a Bloc shouldn't care about how it's being consumed; there are plenty of cases where a UI change would necessitate a change in the states emitted by the Bloc.
As an example, say you had a widget which started in an initial state, then went into a loading state, then showed a SnackBar when an error state was emitted. In this case the Bloc would need to emit the error state to show the SnackBar, immediately followed by the initial state to take the widget out of the loading state. States would be emitted in this order:
added → LoadingBut then say we wanted to change this so that an error page was shown permanently with a restart button. In that case, I would need to change the Bloc so that it didn't emit the initial state after the error state. In other words, the 4th emission in the list above would need to be removed.
This is a very simple example of a UI change which needs to be reflected in the related Bloc.
I definitely agree with @felangel that if this were to be introduced it would have to be a non-breaking change, which would be pretty difficult to achieve.
I actually spent a bit of time preparing a PR for a non-breaking introduction of side-effects and discovered that Dart doesn't support the approach I wanted to take. I wanted to introduce a SideEffect type argument for Blocs but to somehow make it optional or have it default to void, but Dart doesn't support that kind of thing at the moment.
One option would be to allow Blocs and Cubits to emit any object as a side-effect, but that seems icky and dangerous.
Alternatively, a StrictBloc class could be introduced which required a SideEffect type parameter, with corresponding StrictBlocListener and StrictBlocConsumer classes added to the flutter_bloc library.
I really think a discussion around possible approaches to this would be really helpful, and that side-effect support would be a serious improvement to what is already an extremely good library.
felangel
commented
2 years ago @ptrbrynt thanks for the input! Regarding the example you provided, you should never need to emit an error immediately followed by an initial state. To me that's a sign that the bloc/state needs to be restructured. If you're able to provide a link to a repo containing a real-world use case that illustrates the issue, I'm happy to take a look and potentially open a PR with any suggestions.
I also think it's worth noting that adding a separate stream for "side-effects" is pretty simple and can be done without any changes to the core library see https://github.com/allbrightio/effect_bloc for an example 👍
ptrbrynt
commented
2 years ago @felangel Really appreciate the quick response!
I don't have an example to share right now - all my repos are private! However, I'll try and illustrate.
I, like many folks, like to use sealed unions to represent my states. I think it's a really beneficial pattern which works for most use-cases. An example implemented using freezed:
@freezed
class MyState with _$MyState {
const factory MyState.initial() = _Initial;
const factory MyState.loading() = _Loading;
const factory MyState.error(String message) = _Error;
const factory MyState.success(List data) = _Success;
}The above seems like a very common and popular structure for a state. Of course, you could model it in a slightly different way as a single immutable data class, but I think that comes with its own issues. Using a sealed union provides distinct states and ensures developers are always handling all possible cases.
However, this state structure does mean that if I wanted to display an error message followed by the initial state, I would need to call emit(MyState.error('message')), immediately followed by emit(MyState.initial()).
Thanks for linking that package - I think it's a good illustration of what I'm trying to achieve. I guess my main issue is with BlocListener. I appreciate that it was added in response to developer need (and I use it all the time in my code!), but I do still think it breaks the correctness of the Bloc pattern. Either way, I'm definitely going to continue to look into supporting side-effects as part of the Bloc pattern!
Appreciate you taking the time to engage in discussions like this 😁
felangel
commented
2 years ago @felangel Really appreciate the quick response!
I don't have an example to share right now - all my repos are private! However, I'll try and illustrate.
I, like many folks, like to use sealed unions to represent my states. I think it's a really beneficial pattern which works for most use-cases. An example implemented using freezed:
@freezed class MyState with _$MyState { const factory MyState.initial() = _Initial; const factory MyState.loading() = _Loading; const factory MyState.error(String message) = _Error; const factory MyState.success(List data) = _Success; }The above seems like a very common and popular structure for a state. Of course, you could model it in a slightly different way as a single immutable data class, but I think that comes with its own issues. Using a sealed union provides distinct states and ensures developers are always handling all possible cases.
However, this state structure does mean that if I wanted to display an error message followed by the initial state, I would need to call
emit(MyState.error('message')), immediately followed byemit(MyState.initial()).Thanks for linking that package - I think it's a good illustration of what I'm trying to achieve. I guess my main issue is with
BlocListener. I appreciate that it was added in response to developer need (and I use it all the time in my code!), but I do still think it breaks the correctness of the Bloc pattern. Either way, I'm definitely going to continue to look into supporting side-effects as part of the Bloc pattern!Appreciate you taking the time to engage in discussions like this 😁
No problem! If you have time and are able to share a reproduction sample that’d be great. Based on what you shared, it’s not entirely clear to my why you’d want to emit initial right after error. The initial state is generally there to represent the state before any events have been added. Also, BlocListener is called just once per state change so if you’re in an error state you can show a SnackBar via BlocListener without needing to “reset” the state.
nivisi
commented
2 years ago @felangel imagine bloc consumer does something like this:
listener: (previous, current) {
if (current is Error) {
showErrorSnackBar();
}
},
builder: (context, state) {
if (state is Initial || state is Ready) {
return const NormalBody();
}
return const Loader();
}So if we emit the error state and won't emit anything after it, we'll get a loader in the builder. Of course, we can provide buildWhen and exclude the error state there. But it breaks the idea you mentioned in the first answer:
A bloc/cubit manage a single piece of state which can change over time. <...> the bloc shouldn't make assumptions about how the presentation layer will react to the state change.
So in order to perform a navigation or display a snackbar or sth, the presentation or the bloc itself should apply additional effort to avoid unneeded rebuilds or to not leave the UI in an unwanted state.
nivisi
commented
2 years ago cc @ptrbrynt
adrianflutur
commented
2 years ago @nivisi Hi and sorry for interfering, but I also hit this wall of side-effects when I first started to use flutter_bloc and, after I read some code from this issue, it seems that the use-cases presented here are rather ambiguous and could be solved just by rethinking the flow a bit and using the built-in BlocListener. For example, one could try to merge all state classes in one single state class with nullable fields and an enum to keep the current state (that's what I do).
Anyway, if there is a real need to fire this sort of imperative side effects from a BLoC, then sure go ahead. Just add a StreamController in your BLoC, listen to its stream from your widget and fire events from the BLoC when needed. Done.
However, there's one thing to keep in mind when triggering UI side-effects (navigation, show a popup etc) that are not state-driven (especially when working with loading dialogs) : They are fire and forget type of things.. Once fired, imperative methods such as showDialog() or Navigator.of(context).push() are out of your control and you do not know anything about their state (if are they visible or not - unless you await them, but that's not going to happen everywhere).
Here is an example of what I mean by this:
Suppose you have a method in your BLoC that shows a loading dialog before doing some async work, and then it hides the dialog after work is done.
Future<void> onSomeEvent(....) async {
emitEffect(ShowLoadingDialog());
await doSomeWork();
emitEffect(HideLoadingDialog());
}And suppose that in some stateful widget (where this BLoC is being used) the handlers for theese effects do something like this when they are triggered from the BLoC:
@override
void initState() {
super.initState();
onEffect<MyBloc, ShowLoadingDialog>((effect) {
showDialog(context: context, builder: (context) => MyLoadingDialog());
});
onEffect<MyBloc, HideLoadingDialog>((effect) {
Navigator.of(context).pop();
});
}This seems fine and happy right? Well not really, because while doSomeWork executes, other event handler may call emitEffect(HideLoadingDialog()) and close the loading dialog before our handler finished work.
This would mean that our HideLoadingDialog effect is being called twice, which means that not only the loading dialog got popped of the screen too early, but our entire screen just got popped and all we see now is a black screen (extreme case I know, but it's very possible). And the thing is, there is nothing you can do about it since you have no state which tells you if a dialog is above the current screen or not. I never understood why the flutter team decided to add theese imperative calls in the first place, but nevermind that.
Now, from what I've tried so far there are only 2 acceptable ways of doing this:
class BlocState {
PopupState popupState = PopupState.none;
....
}enum PopupState { none, selectAddressPopup, confirmPaymentPopup, .... }
Now I use a `Stack` to show the coresponding popup over the screen content by using `popupState`.
```dart
Widget build(BuildContext context) {
final popupState = context.select((MyBloc bloc) => bloc.state.popupState);
return Stack(
children: [
MyPageContent(),
if (popupState == PopupState.selectAddressPopup)
MySelectSelectAddressPopup(),
if (popupState == PopupState.confirmPaymentPopup)
MyConfirmPaymentPopup(),
....
],
);
}See how my popups are now strongly tied to my state and I know exactly when and what popup is active on the screen. If I would change the state of my bloc and set popupState to none, then no popup would be shown.
Example:
class NavigationExecutor {
const NavigationExecutor({required this.showSelectAddressPopup});
final Future<UserAddress?> Function() showSelectAddressPopup;
}late final NavigationExecutor navigationExecutor;
@override
void initState() {
super.initState();
navigationExecutor = NavigationExecutor(
showSelectAddressPopup: () => showDialog<UserAddress>(
context: context,
builder: (context) => MySelectAddressPopup(),
),
);
}And then pass this executor to your BLoC and call it's method when needed
class MyBloc {
MyBloc(this.navigationExecutor);
final NavigationExecutor navigationExecutor;
Future<void> onSomeEvent(...) async {
emit(...);
final someResult = await doSomeWork();
final userAddress = await navigationExecutor.showSelectAddressPopup();
if (userAddress == null) ....
....
}
}By awaiting the side effect (the popup) we basically know when it has been closed since the code would go past the await when this happens (its better to use the first method though since it can handle many other side effect cases and, well, its state-driven).
Both methods can handle side effects, but the second method might be a little hard to test since you would need to make a mock implementation of the navigation executor and maybe some other things. Good luck
nivisi
commented
2 years ago @adrianflutur hi!
Thanks for your example. I have never thought about doing something like this with side effects. E.g. doing Navigator.pop on some effect seems to be very dangerous just as you described.
For example, one could try to merge all state classes in one single state class with nullable fields and an enum to keep the current state (that's what I do).
Yep. I have mentioned it as a possible solution in the first comment. However, it's not good in my mind. Using th photo example again, it's never possible (by the idea) for the status to be PhotoStatus.selected and for the data to be null at the same time. The selected status is always emitted along with a non-nullable data. But we are forced to define data as nullable. Sounds weird.
For me it looks like the status in this case is redundant and is only needed for side effects, but not for the UI itself. Because we can simply check if (state.data != null) to display the picture.
So, coming back to the initial question, I now think again that side effects and UI states are different and should be separated. For instance, with a stream controller, as you said. Or using that package that @bartekpacia mentioned.
felangel
commented
2 years ago @felangel imagine bloc consumer does something like this:
listener: (previous, current) { if (current is Error) { showErrorSnackBar(); } }, builder: (context, state) { if (state is Initial || state is Ready) { return const NormalBody(); } return const Loader(); }So if we emit the error state and won't emit anything after it, we'll get a loader in the builder. Of course, we can provide
buildWhenand exclude the error state there. But it breaks the idea you mentioned in the first answer:A bloc/cubit manage a single piece of state which can change over time. <...> the bloc shouldn't make assumptions about how the presentation layer will react to the state change.
So in order to perform a navigation or display a snackbar or sth, the presentation or the bloc itself should apply additional effort to avoid unneeded rebuilds or to not leave the UI in an unwanted state.
In this case you can model your state like:
enum MyStatus { initial, loading, success, failure };
class MyState {
const MyState({
this.status = MyStatus.initial,
this.value,
this.error,
});
final MyError? error;
final MyValue? value;
final MyStatus status;
}Then your UI would look like:
listener: (context, state) {
if (state.error) showErrorSnackBar();
},
builder: (context, state) {
switch (state.status) {
case MyStatus.initial:
case MyStatus.success:
case MyStatus.failure:
return const MyBody();
case MyStatus.loading:
return const MyLoadingIndicator();
}
}Closing for now since there doesn't appear to be any actionable next steps here. Feel free to comment with any additional thoughts and I'm happy to continue the conversation 👍
Hello everyone. First of all, thank you for a great package!
I'm new to the bloc pattern, and I have a conceptual question about how
buildersandlistenerswork.TLDR
Why should we emit states for both building the UI (
builders) and for navigation and showing dialogs (listeners)?The problem
Imagine we have a screen where a user can select or take a photo from the device and then upload it to the backend. There are two buttons on that screen:
Some requirements
Stories
Cubit States
Now as we determined stories, we can list states of our cubit:
The code
Good. Now we're ready to implement it. Skipping the details, we have the following classes.
Photo Cubit
photo_cubit.dart
```dart import 'dart:typed_data'; import 'package:bloc/bloc.dart'; import 'package:freezed_annotation/freezed_annotation.dart'; import 'package:gems/domain/usecases/image_picker/pick_photo_from_gallery_use_case.dart'; import 'package:gems/domain/usecases/image_picker/take_photo_use_case.dart'; part 'photo_cubit_state.dart'; part 'photo_cubit.freezed.dart'; class PhotoCubit extends CubitPhoto Cubit State
photo_cubit_state.dart
```dart part of 'photo_cubit.dart'; enum PhotoSelectionErrorType { permissionDenied, permissionPermanentlyDenied, other, } enum PhotoUploadingErrorType { connection, other, } @freezed class PhotoCubitState with _$PhotoCubitState { const factory PhotoCubitState.initial() = _Initial; const factory PhotoCubitState.selecting() = _Selecting; const factory PhotoCubitState.selectionFailed( PhotoSelectionErrorType type, ) = _SelectionFailed; const factory PhotoCubitState.selected( Uint8List data, ) = _Selected; const factory PhotoCubitState.uploading() = _Uploading; const factory PhotoCubitState.uploaded() = _Uploaded; const factory PhotoCubitState.uploadingFailed( PhotoUploadingErrorType type, ) = _UploadingFailed; } ```Photo Screen
photo_screen.dart
```dart import 'package:flutter/material.dart'; import 'package:flutter_bloc/flutter_bloc.dart'; import 'photo_cubit.dart'; class PhotoScreen extends StatelessWidget { const PhotoScreen({Key? key}) : super(key: key); @override Widget build(BuildContext context) { return BlocListenerMy questions
So far it seems to be good. We use listeners to show alerts on errors. We use builders to render the UI.
Some states are for the UI, some states are for the listeners. Aren't they conceptually different?
What can be seen here, for builders we only use
InitialandSelectedstates whereas for listeners we use all the other states. This brought me to the initial question: why should states likePermissionDeniedbe an actual state of a cubit, and not kind of an event?How to correctly persist the data of
Selectedstate if another one is emitted?The scenario is:
Selectedstate);Selected— but it is expected for the state to beSelected.We can:
Make
datato be shareable, but nullable in all the states. So in theuploadmethod we can check if thedatais not null and try to upload it. In the UI we can also check if it is not null and then render it and make the upload button enabled.But then:
Selectedstate as we rely on the data field only? So we don't need to check if state isSelected, we only need to check ifdatais not null;Selectedcan only be emitted when there isdata, whydatashould ever be nullable?Emit the
Selectedstate right after we fire any "event" state likePermissionDenied. But it seems kind of... redundant?Split these cubits in two. One is for the UI state, one is for the events. But what’s the point to have an option to use listeners in the bloc for the UI and use builders in the bloc for the listeners?
Maybe I’m misunderstanding the idea of bloc here, but doesn’t it seem logical to have kind of events in cubits along with the states? So, if we need to navigate/show alert/etc, we should fire an event for the listeners. If we need to build the UI, we should emit a state for the builders.
Thank you. And sorry for a long read 😀 I would like to hear your opinion on that and — if possible — your solution to that screen.