Closed felangel closed 3 years ago
One other thought I had; today mapEventToState
is fairly permissive in terms of handling or not handling events. If you don't handle an event, and you're using async*
syntax, chances are it defaults to no-op.
Personally I always prefer to treat unhandled events as an Error
since it seems like would (almost?) always be a programmer mistake to allow events to be dispatched to a bloc that no handler is registered for. If you want to ignore an event it feels like you'd at least want to be explicit about it.
With this API change, is there a possibility to make error the default behavior of unhandled events? Or at least easily opt-in for that?
One other thought I had; today
mapEventToState
is fairly permissive in terms of handling or not handling events. If you don't handle an event, and you're usingasync*
syntax, chances are it defaults to no-op.Personally I always prefer to treat unhandled events as an
Error
since it seems like would (almost?) always be a programmer mistake to allow events to be dispatched to a bloc that no handler is registered for. If you want to ignore an event it feels like you'd at least want to be explicit about it.With this API change, is there a possibility to make error the default behavior of unhandled events? Or at least easily opt-in for that?
Yeah definitely! With the proposed API it should be straightforward to throw an Error if there is no handler registered π
Personally I always prefer to treat unhandled events as an
Error
since it seems like would (almost?) always be a programmer mistake to allow events to be dispatched to a bloc that no handler is registered for. If you want to ignore an event it feels like you'd at least want to be explicit about it.
Agree. I think this is one of the reasons for huge popularity of freezed
and similar packages that, even tough they increase the cognitive load, they also simplify handling all the events or just the ones that we care of with the when()
syntax:
yield event.when(
increment: () => CounterState.current(state.value + 1),
decrement: () => CounterState.current(state.value - 1),
);
Having similar possibility without introducing new dependencies or code generation would be definitely a huge improvement.
@orestesgaolin Yeah, I totally agree. The ideal scenario is that built-in dartlang
features will someday allow us to be exhaustive about our cases so forgetting to handle one is an analyze-time error, but without complicated code generation dependencies to support it. But I think this API would be good for the way things are today, and can still support aftermarket when
implementations if needed.
I'm in favor of this change. My biggest concern, as always, is the migration path.
The way blocs are used and tested will be 100% backward compatible which means the changes will be scoped to just the mapEventToState code and can ideally be automated via a code mod.
I'd feel much better about this change if you could help author a code mod for this. This is especially important for teams using freezed
to map events. For example, the codebase I primarily work in has 91 Bloc
s. So, converting them by hand is feasible but would be unpleasant.
I'm in favor of this change. My biggest concern, as always, is the migration path.
The way blocs are used and tested will be 100% backward compatible which means the changes will be scoped to just the mapEventToState code and can ideally be automated via a code mod.
I'd feel much better about this change if you could help author a code mod for this. This is especially important for teams using
freezed
to map events. For example, the codebase I primarily work in has 91Bloc
s. So, converting them by hand is feasible but would be unpleasant.
Thanks for the feedback! Yeah I agree a code mod would be included as part of these changes to aid in the migration of larger projects π
Writing blocs requires an understanding of Streams and async generators. This means developers must understand how to use the async, yield, and yield keywords. While these concepts are covered in the documentation, they are still fairly complex and difficult for newcomers to grasp.
This will go a long way in making bloc
more accessible to new developers.
My only concern would be the amount of refactoring involved for larger projects.
Do you have a recommended migration path in mind?
Writing blocs requires an understanding of Streams and async generators. This means developers must understand how to use the async, yield, and yield keywords. While these concepts are covered in the documentation, they are still fairly complex and difficult for newcomers to grasp.
This will go a long way in making
bloc
more accessible to new developers.My only concern would be the amount of refactoring involved for larger projects.
Do you have a recommended migration path in mind?
Thanks for the feedback! π
Do you have a recommended migration path in mind?
If we decide to move forward with the proposal I intend to ship a migration tool to aid in the migration π
@felangel For migration on a large codebase, I opened an issue on Flutter repo to open dart migrate
and flutter fix
to libraries maintainers
https://github.com/flutter/flutter/issues/78735
Would like to get more thump ups guys
I landed here because we hit the generators issue you referenced. If this will solve it, I think it's great. We can relate to the big boilerplate problem as well.
One other 'personal' preference is maybe to use setState(...)
instead of emit(...)
?
Considering BLoC's are used for StateManagement and you are kind of setting the active state, it feels like it makes sense.
I know bloc isn't for Flutter only, but for people coming from Flutter (which I bet is the majority) it should be very relatable cause of the StatefullWidgets
.
I landed here because we hit the generators issue you referenced. If this will solve it, I think it's great. We can relate to the big boilerplate problem as well.
One other 'personal' preference is maybe to use
setState(...)
instead ofemit(...)
? Considering BLoC's are used for StateManagement and you are kind of setting the active state, it feels like it makes sense. I know bloc isn't for Flutter only, but for people coming from Flutter (which I bet is the majority) it should be very relatable cause of theStatefullWidgets
.
That's great to hear, thanks for the feedback! Regarding setState
, the reason I prefer to avoid it is because the signature is different than what developers are used to with StatefulWidget
(there is no lambda with mutations). I also was trying to convey that the new state change will notify listeners and also wanted to make the API more consistent with Cubit
. Let me know what you think and thanks again for the feedback! π
That's great to hear, thanks for the feedback! Regarding
setState
, the reason I prefer to avoid it is because the signature is different than what developers are used to withStatefulWidget
(there is no lambda with mutations). I also was trying to convey that the new state change will notify listeners and also wanted to make the API more consistent withCubit
. Let me know what you think and thanks again for the feedback! π
I don't see the big issue in the signatures being different. But now that you want to get rid of mapEventTo*State*
, setState
might not be that helpful. ^_^'
Predictability
Due to an issue in Dart, it is not always intuitive what the value of
state
will be when dealing with nested async generators which emit multiple states. Even though there are ways to work around this issue, one of the core principles/goals of the bloc library is to be predictable. Therefore, the primary motivation of this proposal is to make the library as safe as possible to use and eliminate any uncertainty when it comes to the order and value of state changes.
Hi there @felangel. A few months ago I tried to to something similar with your package, but I failed because my solution needed reflection and Flutter is actually very poor on that theme. But, the thing is, I wanted to make Bloc like a DFA, creating deterministic transitions like any other automaton but without breacking all the code written with previus versions. Now, your solution is by far more elegant and simpler than mine, but, have you considered including the currentState in transitions? For example:
abstract class CounterEvent {} class Increment extends CounterEvent {} class Decrement extends CounterEvent {} abstract class CounterState {} class PreviouslyIncremented extends CounterState {} class PreviouslyDecremented extends CounterState {} class CounterBloc extends Bloc<CounterEvent, CounterState> { CounterBloc() : super(0) { on<Increment, PreviouslyDecremented >((event, state, emit) => emit(state + 1)); on<Decrement, PreviouslyIncremented>((event, state, emit) => emit(state - 1)); } }
This might be very helful when you want to simulate many complex logics. Some times you want to have some invalid transitions(like you can't go to an Error state without passing through a Loading state), so now you need to test that transition never happends. The current solution is to ensure that you are currently in the desired state, but this can get tricky when you have to many states to handle. Get's worst defining a final state. Now, we need to test for every event in Bloc, that nothing will happpend after we reach it. Would be cool to define a way very elegant like you did, that fully describes the flux of the bloc.
Now, I will like to know how you will handle inheritance beetween events, because there is where I got stocked. It's not a really good practice to share data beetween events, but I'm aware that many people are using it, and I wanted to be fully compatible. Thank you for your time and congrats for your work, bloc is the best.
@felangel , how do you propose to manage the event queue without transformEvents
?
Though I find new on
helpful, if you don't use freezed
, I totally going to say no for turning emitting functions from Stream
ones to void-emit
just because some persons won't able to figure out how work out things that language provides to them, most of these situations though caused by the fact they don't even tried to learn it, I think you must have noticed it by the moment.
And not be missing that point, that it will appear in they coding experience anyway in a near perspective.
I find this "improvement" as a local surrogate of broad language-wide feature, thus making it complete nonsense to apply to the lib, as it is not making any difference in shrinking the code too.
And why you need on in constructor, if you can made much simple:
import 'package:bloc/bloc.dart';
/// BLoC with complex events, containing internal data.
///
/// Override [router] and create generators.
///
/// Example usage:
/// class MyRouterBloc extends RouterBloc<Event, State> {
/// MyRouterBloc() : super(InitialState());
///
/// @override
/// Map<Type, Function> get router =>
/// <Type, Function>{
/// PerformEvent : _perform,
/// };
///
/// Stream<State> _perform(PerformEvent event) async* {
/// yield PerformingState();
/// // ...
/// yield PerformedState();
/// }
/// }
///
abstract class RouterBloc<Event, State> extends Bloc<Event, State> {
/// BLoC with complex events, containing internal data.
RouterBloc(State initialState) : super(initialState);
Map<Type, Function> _routerCache;
Map<Type, Function> _internalRouter() => _routerCache ??= router;
/// Sets the generator router by event type
///
/// @override
/// Map<Type, Function> get router =>
/// <Type, Function>{
/// CreateEvent : _createStateGenerator,
/// ReadEvent : _readStateGenerator,
/// UpdateEvent : _updateStateGenerator,
/// DeleteEvent : _deleteStateGenerator,
/// }
Map<Type, Function> get router;
@override
Stream<State> mapEventToState(Event event) async* {
final type = event.runtimeType;
final internalRouter = _internalRouter();
assert(
internalRouter.containsKey(type),
'router in RouterBloc must contain $type key',
);
if (!internalRouter.containsKey(type)) return;
yield* internalRouter[type](event) as Stream<State>;
}
}
Why do you want to make it a breaking change? Why not simply implement mapEventToState
with on<...>
and whoever wants to work with mapEventToState
will simply override the method?
I like this change, I don't mind it but I'd prefer keeping the mapEventToState
and not making another breaking change.
Looking at the changelog at pub, the package has had a lot of breaking changes (more or less, one per major version) and so it doesn't look very stable. But don't get me wrong! The quality of the package & ecosystem is one of the best in my opinion, but the "unstable" part is related to the frequent breaking changes.
One thing I don't like about this proposal is that it actually comprises of many minor proposals which are completely independent. For instance, most of people here like how interface is improved with on<...>
syntax. However, getting rid of events stream as a fix for the bug is a questionable decision and I feel that it needs further discussion.
I would rather separate these two proposals.
Hi @felangel , I met with this issue from version 5 If i am not mistaken (When started use your library) But I solve this issue easily
typedef State = int;
final stateGenerator = bad;
void main() => stateGenerator().map<State>((i) => state = i).listen(print);
State state = 0;
Stream<State> bad() async* {
Stream<State> _inline() async* {
var newState=state; // working when use reference or adding microtask
newState= state + 1;
yield newState;
newState=newState + 1;
yield newState;
}
yield* _inline();
}
I am afraid of that many developers need refactoring their code after update to new version of bloc ver. 8
It costs quite a few resources.
Moreover, we lose customization of transformEvent
and many courses articles have been already written and many newbies in flutter will have problem in the use of this state management because they will look and study from course and articles (They do not like read new docs). Moreover, many not fool issues will be open.
I hope that you will pay attentions to this moments:)
New update trying to fix https://github.com/dart-lang/sdk/issues/44616
Simple demonstration of bug: https://dartpad.dev/8fe1bb47b6b5a6a496ced110f426c3a6
Workaround (if someone need quickfix in current project):
Stream<State> mapEventToState(Event _) async* {
Stream<State> _inline() async* {
yield state + 1;
yield state + 1;
}
// Problem:
//yield* _inline();
// Workaround:
yield* _inline().asyncMap<State>(
(i) => Future<State>.microtask(() => i),
);
}
Why do you want to make it a breaking change? Why not simply implement
mapEventToState
withon<...>
and whoever wants to work withmapEventToState
will simply override the method?
The reason is because keeping mapEventToState
in the current state is susceptible to the dart issue mentioned in the proposal.
I like this change, I don't mind it but I'd prefer keeping the
mapEventToState
and not making another breaking change.Looking at the changelog at pub, the package has had a lot of breaking changes (more or less, one per major version) and so it doesn't look very stable. But don't get me wrong! The quality of the package & ecosystem is one of the best in my opinion, but the "unstable" part is related to the frequent breaking changes.
If there weren't breaking changes there wouldn't be major version bumps haha. Following semantic versioning, major versions indicate breaking changes so that developers can choose to migrate at their own pace.
Thanks for the positive feedback, hope that helps.
Hi @felangel , I met with this issue from version 5 If i am not mistaken (When started use your library) But I solve this issue easily
typedef State = int; final stateGenerator = bad; void main() => stateGenerator().map<State>((i) => state = i).listen(print); State state = 0; Stream<State> bad() async* { Stream<State> _inline() async* { var newState=state; // working when use reference or adding microtask newState= state + 1; yield newState; newState=newState + 1; yield newState; } yield* _inline(); }
I am afraid of that many developers need refactoring their code after update to new version of bloc ver. 8 It costs quite a few resources. Moreover, we lose customization of
transformEvent
and many courses articles have been already written and many newbies in flutter will have problem in the use of this state management because they will look and study from course and articles (They do not like read new docs). Moreover, many not fool issues will be open.I hope that you will pay attentions to this moments:)
Thanks for the feedback! Yes, you can definitely work around the issue but it's not ideal. As mentioned above, if these changes are implemented there would also be a codemod which helps you migrate your existing projects. In addition, you will still be able to customize concurrency (and it will be easier imo) because the library will ship with built-in concurrency modes (enqueue, concurrent, drop, debounceTime, etc...).
Regarding outdated documentation and tutorials, that is always a concern. If this proposal lands, it will include updating all documentation and working with content creators to create updated tutorials, examples, etc. In addition, since it would be a major version bump, developers can stay on v7.x.x for as long as they like and migrate when they are ready. Hope that helps π
One thing I don't like about this proposal is that it actually comprises of many minor proposals which are completely independent. For instance, most of people here like how interface is improved with
on<...>
syntax. However, getting rid of events stream as a fix for the bug is a questionable decision and I feel that it needs further discussion.I would rather separate these two proposals.
The primary goal of this proposal is to address the predictability issue which can occur due to this. In my opinion, the two are not completely independent because in order to solve the primary issue we would need to change the signature of mapEventToState
to look something like:
Stream<void> mapEventToState(Event event, Emitter<State> emit) async* {...}
This naming no longer makes sense in my opinion (we aren't mapping events to states because the return type is Stream<void>
) and it is already a breaking change as is -- hence the proposal to replace mapEventToState
with on<E>
. Do you prefer to keep the name mapEventToState
? Thanks for the feedback! π
@felangel , how do you propose to manage the event queue without
transformEvents
?
on<E>
would accept an EventModifier
and the library would ship with several built-in modifiers including:
concurrent
, enqueue
, restartable
, keepLatest
, drop
, and debounceTime
.
on<Increment>(
(event, emit) => emit(state + 1),
debounceTime(const Duration(seconds: 1)),
);
Still working through this piece but hopefully that answers your question.
Though I find new
on
helpful, if you don't usefreezed
, I totally going to say no for turning emitting functions fromStream
ones tovoid-emit
just because some persons won't able to figure out how work out things that language provides to them, most of these situations though caused by the fact they don't even tried to learn it, I think you must have noticed it by the moment.And not be missing that point, that it will appear in they coding experience anyway in a near perspective.
I find this "improvement" as a local surrogate of broad language-wide feature, thus making it complete nonsense to apply to the lib, as it is not making any difference in shrinking the code too.
Thanks for the feedback. I'm not sure I fully understand -- the goal of this change is to make the library safer to use. The core issue is with the timing of how nested async generators work and it is not guaranteed that this will ever be changed in the Dart language (see https://github.com/dart-lang/sdk/issues/44616#issuecomment-851316899).
Hi @felangel , I met with this issue from version 5 If i am not mistaken (When started use your library) But I solve this issue easily
typedef State = int; final stateGenerator = bad; void main() => stateGenerator().map<State>((i) => state = i).listen(print); State state = 0; Stream<State> bad() async* { Stream<State> _inline() async* { var newState=state; // working when use reference or adding microtask newState= state + 1; yield newState; newState=newState + 1; yield newState; } yield* _inline(); }
I am afraid of that many developers need refactoring their code after update to new version of bloc ver. 8 It costs quite a few resources. Moreover, we lose customization of
transformEvent
and many courses articles have been already written and many newbies in flutter will have problem in the use of this state management because they will look and study from course and articles (They do not like read new docs). Moreover, many not fool issues will be open.I hope that you will pay attentions to this moments:)
Thanks for the feedback! Yes, you can definitely work around the issue but it's not ideal. As mentioned above, if these changes are implemented there would also be a codemod which helps you migrate your existing projects. In addition, you will still be able to customize concurrency (and it will be easier imo) because the library will ship with built-in concurrency modes (enqueue, concurrent, drop, debounceTime, etc...).
Regarding outdated documentation and tutorials, that is always a concern. If this proposal lands, it will include updating all documentation and working with content creators to create updated tutorials, examples, etc. In addition, since it would be a major version bump, developers can stay on v7.x.x for as long as they like and migrate when they are ready. Hope that helps π
Thanks @felangel By the way, what if new type of concurrency will be needed in specific use case? Will built-in concurrency modes (enqueue, concurrent, drop, debounceTime, etc...) be customizable in version 8.x.x?
Hi @felangel , I met with this issue from version 5 If i am not mistaken (When started use your library) But I solve this issue easily
typedef State = int; final stateGenerator = bad; void main() => stateGenerator().map<State>((i) => state = i).listen(print); State state = 0; Stream<State> bad() async* { Stream<State> _inline() async* { var newState=state; // working when use reference or adding microtask newState= state + 1; yield newState; newState=newState + 1; yield newState; } yield* _inline(); }
I am afraid of that many developers need refactoring their code after update to new version of bloc ver. 8 It costs quite a few resources. Moreover, we lose customization of
transformEvent
and many courses articles have been already written and many newbies in flutter will have problem in the use of this state management because they will look and study from course and articles (They do not like read new docs). Moreover, many not fool issues will be open. I hope that you will pay attentions to this moments:)Thanks for the feedback! Yes, you can definitely work around the issue but it's not ideal. As mentioned above, if these changes are implemented there would also be a codemod which helps you migrate your existing projects. In addition, you will still be able to customize concurrency (and it will be easier imo) because the library will ship with built-in concurrency modes (enqueue, concurrent, drop, debounceTime, etc...). Regarding outdated documentation and tutorials, that is always a concern. If this proposal lands, it will include updating all documentation and working with content creators to create updated tutorials, examples, etc. In addition, since it would be a major version bump, developers can stay on v7.x.x for as long as they like and migrate when they are ready. Hope that helps π
Thanks @felangel By the way, what if new type of concurrency will be needed in specific use case? Will built-in concurrency modes (enqueue, concurrent, drop, debounceTime, etc...) be customizable in version 8.x.x?
Yes, you'll be able to define your own concurrency modes. I'm still in the process of working out the interfaces for everything but I'll share more info soon π
@II11II are you still against the proposal and if so can you provide your reasoning? Thanks π
And why you need on in constructor, if you can made much simple:
import 'package:bloc/bloc.dart'; /// BLoC with complex events, containing internal data. /// /// Override [router] and create generators. /// /// Example usage: /// class MyRouterBloc extends RouterBloc<Event, State> { /// MyRouterBloc() : super(InitialState()); /// /// @override /// Map<Type, Function> get router => /// <Type, Function>{ /// PerformEvent : _perform, /// }; /// /// Stream<State> _perform(PerformEvent event) async* { /// yield PerformingState(); /// // ... /// yield PerformedState(); /// } /// } /// abstract class RouterBloc<Event, State> extends Bloc<Event, State> { /// BLoC with complex events, containing internal data. RouterBloc(State initialState) : super(initialState); Map<Type, Function> _routerCache; Map<Type, Function> _internalRouter() => _routerCache ??= router; /// Sets the generator router by event type /// /// @override /// Map<Type, Function> get router => /// <Type, Function>{ /// CreateEvent : _createStateGenerator, /// ReadEvent : _readStateGenerator, /// UpdateEvent : _updateStateGenerator, /// DeleteEvent : _deleteStateGenerator, /// } Map<Type, Function> get router; @override Stream<State> mapEventToState(Event event) async* { final type = event.runtimeType; final internalRouter = _internalRouter(); assert( internalRouter.containsKey(type), 'router in RouterBloc must contain $type key', ); if (!internalRouter.containsKey(type)) return; yield* internalRouter[type](event) as Stream<State>; } }
Thanks for the feedback! This approach doesn't take into consideration inheritance. With on<T>
when an event of type E
is added, every handler where T
is E
will be invoked. In addition, by retaining mapEventToState
we are not addressing the primary goal of this proposal which is to make the library more predictable by eliminating the potential pitfall with nested async generators.
Hi @felangel, i really like the approach! Even though i just recently started using bloc, it does make it more readable/understandable! I now already started using this way in my personal development branches :)
I have one question regarding the implementation:
why do you provide emit
as parameter?
on my projects it works, even if i rename this to emit_dummy and use emit (probably from the base class)
abstract class CounterEvent {}
class Increment extends CounterEvent {}
class CounterBloc extends Bloc<CounterEvent, int> {
CounterBloc() : super(0) {
// on<Increment>((event, emit) => emit(state + 1));
on<Increment>((event, emit_dummy) => emit(state + 1)); // works as well
}
}
Wouldn't this work as well? It seems that Emit declaration is unecessary...
abstract class CounterEvent {}
class Increment extends CounterEvent {}
class Decrement extends CounterEvent {}
class CounterBloc extends Bloc<CounterEvent, int> {
CounterBloc() : super(0) {
on<Increment>(_onIncrement);
on<Decrement>(_onDecrement);
}
void _onIncrement(Increment event) {
emit(state + 1);
}
void _onDecrement(Decrement event) {
emit(state - 1);
}
}
Edit: Is this related to this issue:
Hi @felangel, i really like the approach! Even though i just recently started using bloc, it does make it more readable/understandable! I now already started using this way in my personal development branches :)
I have one question regarding the implementation:
why do you provide
emit
as parameter? on my projects it works, even if i rename this to emit_dummy and use emit (probably from the base class)abstract class CounterEvent {} class Increment extends CounterEvent {} class CounterBloc extends Bloc<CounterEvent, int> { CounterBloc() : super(0) { // on<Increment>((event, emit) => emit(state + 1)); on<Increment>((event, emit_dummy) => emit(state + 1)); // works as well } }
Wouldn't this work as well? It seems that Emit declaration is unecessary...
abstract class CounterEvent {} class Increment extends CounterEvent {} class Decrement extends CounterEvent {} class CounterBloc extends Bloc<CounterEvent, int> { CounterBloc() : super(0) { on<Increment>(_onIncrement); on<Decrement>(_onDecrement); } void _onIncrement(Increment event) { emit(state + 1); } void _onDecrement(Decrement event) { emit(state - 1); } }
Edit: Is this related to this issue:
560
?
Thanks for the feedback! You shouldn't be able to use emit
directly because it is annotated with visibleForTesting
in order to ensure that all state changes occur in response to an event. If emit
could be called directly then there wouldn't be anything stopping developers from doing something like:
class MyBloc extends Bloc<...> {
void foo() {
emit(...);
}
}
which would mean that state changes can occur without a corresponding event.
Hope that helps π
Hi @felangel ,
Thanks for the feedback! You shouldn't be able to use
emit
directly because it is annotated withvisibleForTesting
in order to ensure that all state changes occur in response to an event. Ifemit
could be called directly then there wouldn't be anything stopping developers from doing something like:class MyBloc extends Bloc<...> { void foo() { emit(...); } }
which would mean that state changes can occur without a corresponding event.
thanks for the reply. I actually don't see the problem here.
Everything that happens in the bloc, stays within the blocs context. i currently use (hopefully not misuse) the concept like this:
void _onUpdate(
_MyUpdate event,
Emit<MyState> emit,
) async {
try {
emit(MyState.updateInProgress(0));
var result = 0;
for (var x=0; x < event.max; x++) {
result += await _update(x);
emit(MyState.updateInProgress(x/event.max*100));
}
emit(MyState.updateFinished(result));
} catch (e) {
emit(MyState.failure(e));
}
}
i have several state changes in one event handling function, to notify the user/UI about the current progress of an operation. Even if such operation is handled in a seperate background thread - it all happens inside the Bloc's context.
Am I misunderstanding the concept? Should there always be a 1-to-1 relationship between Events & StateChanges? Technically both are different streams and can just be filled - can't they?
And if you think of Cubits - this is just happening - isn't it? You just define a function and call emit() to raise a state.
Am I missing something?
Hi @felangel ,
Thanks for the feedback! You shouldn't be able to use
emit
directly because it is annotated withvisibleForTesting
in order to ensure that all state changes occur in response to an event. Ifemit
could be called directly then there wouldn't be anything stopping developers from doing something like:class MyBloc extends Bloc<...> { void foo() { emit(...); } }
which would mean that state changes can occur without a corresponding event.
thanks for the reply. I actually don't see the problem here.
Everything that happens in the bloc, stays within the blocs context. i currently use (hopefully not misuse) the concept like this:
void _onUpdate( _MyUpdate event, Emit<MyState> emit, ) async { try { emit(MyState.updateInProgress(0)); var result = 0; for (var x=0; x < event.max; x++) { result += await _update(x); emit(MyState.updateInProgress(x/event.max*100)); } emit(MyState.updateFinished(result)); } catch (e) { emit(MyState.failure(e)); } }
i have several state changes in one event handling function, to notify the user/UI about the current progress of an operation. Even if such operation is handled in a seperate background thread - it all happens inside the Bloc's context.
Am I misunderstanding the concept? Should there always be a 1-to-1 relationship between Events & StateChanges? Technically both are different streams and can just be filled - can't they?
And if you think of Cubits - this is just happening - isn't it? You just define a function and call emit() to raise a state.
Am I missing something?
The problem is with bloc every state change should be in response to an Event
. With cubit, this is not the case -- state changes occur in response to function calls so there is no concept of events. In order to ensure that emit
is only called in response to an event which is added emit
is only available in response to events in the on
handler. If we allow emit
to be used globally then there is nothing preventing developers from calling emit
sporadically in response to other inputs which would result in state changes (transitions) that aren't associated with an event. Hope that makes sense π
@felangel what about case when you use enum in event type? Will it cover this case or will it break it?
@felangel what about case when you use enum in event type? Will it cover this case or will it break it?
Enums should still be supported π
enum CounterEvent { increment, decrement }
class CounterBloc extends Bloc<CounterEvent, int> {
CounterBloc() : super(0) {
on<CounterEvent>(_onEvent);
}
void _onEvent(CounterEvent event, Emitter<int> emit) {
switch (event) {
case CounterEvent.decrement:
return emit(state - 1);
case CounterEvent.increment:
return emit(state + 1);
}
}
}
Hey everyone, I'm not going to pretend to understand all the complexities here, especially the ordeal with nestedAsyncGenerators. I just want to add that as a developer this new API truly helps me understand how data is flowing.
This is 10x more understandable
on<AppUserChanged>(_onUserChanged);
...
void _onUserChanged(AppUserChanged event, Emitter<AppState> emit) {
emit(event.user.isNotEmpty
? AppState.authenticated(event.user)
: const AppState.unauthenticated());
}
Than this
@override
Stream<AppState> mapEventToState(AppEvent event) async* {
if (event is AppUserChanged) {
yield _mapUserChangedToState(event, state);
} else if (event is AppLogoutRequested) {
unawaited(_authenticationRepository.logOut());
}
}
AppState _mapUserChangedToState(AppUserChanged event, AppState state) {
return event.user.isNotEmpty
? AppState.authenticated(event.user)
: const AppState.unauthenticated();
}
Beyond that, just the params of event and emit complements cubit so well..
All in all, semantically speaking, this helps me visualize what's happening which in turn helps me develop, debug, and deliver faster. To me, that's the goal !
It looks amazing!
One thing that I am interested in, how will this change integrate with Freezed's unions? Before, it was possible to describe the mapping of single emissions as:
Stream<int> mapEventToState(CounterEvent event) => Stream.value(
event.when(
increment: (amount) => state.copyWith(value: state + amount),
decrement: (amount) => state.copyWith(value: state - amount),
),
);
It is pretty boilerplate-free. How would it look with the updated API?
It looks amazing!
One thing that I am interested in, how will this change integrate with Freezed's unions? Before, it was possible to describe the mapping of single emissions as:
Stream<int> mapEventToState(CounterEvent event) => Stream.value( event.when( increment: (amount) => state.copyWith(value: state + amount), decrement: (amount) => state.copyWith(value: state - amount), ), );
It is pretty boilerplate-free. How would it look with the updated API?
Thanks so much for the feedback! With the new API it would look like:
class CounterBloc extends Bloc<CounterEvent, CounterState> {
CounterBloc() : super(CounterState(...)) {
on<CounterEvent>((event, emit) {
emit(event.when(
increment: (amount) => state.copyWith(value: state + amount),
decrement: (amount) => state.copyWith(value: state - amount),
),
);
});
}
}
Hope that helps π
It looks amazing! One thing that I am interested in, how will this change integrate with Freezed's unions? Before, it was possible to describe the mapping of single emissions as:
Stream<int> mapEventToState(CounterEvent event) => Stream.value( event.when( increment: (amount) => state.copyWith(value: state + amount), decrement: (amount) => state.copyWith(value: state - amount), ), );
It is pretty boilerplate-free. How would it look with the updated API?
Thanks so much for the feedback! With the new API it would look like:
class CounterBloc extends Bloc<CounterEvent, CounterState> { CounterBloc() : super(CounterState(...)) { on<CounterEvent>((event, emit) { emit(event.when( increment: (amount) => state.copyWith(value: state + amount), decrement: (amount) => state.copyWith(value: state - amount), ), ); }); } }
Hope that helps π
That clears that out, thanks!
@felangel My final suggestion (I think about not having breakchanges) is to create mixin.
class CounterBloc extends Bloc<CounterEvent, CounterState> with BlocMixin {}
The BlocMixin would do the work of the mapEventToState and add the method on
@felangel My final suggestion (I think about not having breakchanges) is to create mixin.
class CounterBloc extends Bloc<CounterEvent, CounterState> with BlocMixin {}
The BlocMixin would do the work of the mapEventToState and add the method on();
Thanks for the feedback! The issue with this is mapEventToState
and on
should not coexist. It should not be possible as a developer to register event handlers via on
and still use mapEventToState
because the way in which concurrency is defined/implemented is incompatible and it would lead to unexpected behavior and potentially even duplicate event handler executions. I'm working on a migration tool which would automatically convert v7.0.0 blocs to the v8.0.0 syntax and I'll provide more updates as I have them. Let me know what you think and thanks again for your feedback/suggestions!
If you have to use immutable states and events, you may use the freezed
packages that have Unions and a map
function.
For me, It's not clear why we have to replace mapEventToState
function with the subscription logic.
In my pet project I have some logic like this:
@freezed
class ScreenEvent with _$ScreenEvent {
factory ScreenEvent.load() = LoadEvent;
factory ScreenEvent.createdNew({required Project project}) = CreatedNewEvent;
}
@freezed
class ScreenState with _$ScreenState {
@freezed
class ScreenEvent with _$ScreenEvent {
factory ScreenEvent.load() = LoadEvent;
factory ScreenEvent.createdNew({required Project project}) = CreatedNewEvent;
}
@freezed
class ScreenState with _$ScreenState {
factory ScreenState.initial() = InitialState;
factory ScreenState.loaded({
required List<Project> projects,
}) = LoadedState;
}
class ScreenBloc extends Bloc<ScreenEvent, ScreenState> {
final ProjectsRepository projectsRepository;
final AppRoutingBloc appRoutingBloc;
late StreamSubscription projectsSub;
ScreenBloc({
required this.projectsRepository,
required this.appRoutingBloc,
}) : super(ScreenState.initial()) {
projectsSub = projectsRepository
.watchForChanges()
.map((_) => ScreenEvent.load())
.listen(add);
}
@override
Future<void> close() {
projectsSub.cancel();
return super.close();
}
@override
Stream<ScreenState> mapEventToState(ScreenEvent event) async* {
yield* event.map(
load: (event) async* {
final list = await projectsRepository.getListByParentId(null);
yield ScreenState.loaded(projects: list);
},
createdNew: (event) async* {
await projectsRepository.createProject(event.project);
appRoutingBloc.add(AppRoutingEvent.openedProject(event.project.id));
},
);
}
}
It solves a lot of boilerplate. If you keep in mind the async stream's bug, that is mentioned by you, we will kill the second reason to do breaking change.
And the last one - complexity. IMHO, dart's streams are part of the framework and the bridge to RxDart and a lot of other libs (I use moor in the sample above). If you did replace streams with some functions like debounce
, it would break compatibility.
In my memory, I've moved from Vue.js to React for one reason: Vue2 was updated to Vue 3 with no breaking changes but with new hooks syntax, exact like in react. Authors divided all written code into two parts: Vue code and some react-like piece of js.
Now you try to divide code on the dart-based bloc and some redux-like piece of dart.
IMHO dart is the one language, that requires the codegen to live. Now, every class is code-generated, and that works well.
That's why, everyone, who needs a pure bloc, can write classes themselves, but those, who need an opportunity to code fast, can use freezed
.
Hope for feedback.
@datdefboi thanks for the feedback!
For me, It's not clear why we have to replace mapEventToState function with the subscription logic.
What do you mean by subscription logic? The proposed changes don't introduce additional subscriptions.
It solves a lot of boilerplate. If you keep in mind the async stream's bug, that is mentioned by you, we will kill the second reason to do breaking change.
Freezed solves one problem and introduces another imo. While you get unions and mappers, you pay the price of relying on code generation which takes increasingly more time as the project grows and hundreds of lines of generated code (often more than you need).
If you keep in mind the async stream's bug, that is mentioned by you, we will kill the second reason to do breaking change.
The async stream bug is the primary reason for this change. The reduced complexity and boilerplate are just benefits we get as a result but if it weren't for the async stream bug I most likely would not have created this proposal.
And the last one - complexity. IMHO, dart's streams are part of the framework and the bridge to RxDart and a lot of other libs (I use moor in the sample above). If you did replace streams with some functions like debounce, it would break compatibility.
We aren't planning to replace streams, blocs will still consist of a single event sink and state stream. You will still be able to use rxdart if you like but the key difference imo is you won't be forced to (especially for one or two transformers). As a developer you can choose whether you want to pull in an external dependency like rxdart and in most cases if it's just because you want to have a debounceTime
or throttleTime
function you won't need to.
IMHO dart is the one language, that requires the codegen to live.
I personally disagree. With the current infrastructure relying heavily on codegen is not sustainable and doesn't scale as project size increases. There are cases where codegen makes sense imo like localizations, json (de)serialization because they are classes that don't change frequently as your feature set grows. In any case, it comes down to the project and team preference so I don't think we should make the assumption that all developers using bloc should also use freezed
.
In my memory, I've moved from Vue.js to React for one reason: Vue2 was updated to Vue 3 with no breaking changes but with new hooks syntax, exact like in react.
As I mentioned before, I acknowledge that breaking changes are disruptive and not ideal -- we only consider them if the change is justifiable over the long-term and are still evaluating and experimenting with potential ways to introduce these improvements with as few breaking changes as possible. If the proposed changes are shipped as described, they will be accompanied by a codemod/migration tool which should handle the majority of the migration for you.
Hope that helps and thanks again for your feedback, I really appreciate you taking the time to provide your thoughts! π
What do you mean by subscription logic? The proposed changes don't introduce additional subscriptions.
That means the new bloc syntax with on
keyword;
Generally, your arguments sound good. What's the expected release date?
What do you mean by subscription logic? The proposed changes don't introduce additional subscriptions.
That means the new bloc syntax with
on
keyword;Generally, your arguments sound good. What's the expected release date?
Ah thanks for clarifying. Hopefully https://github.com/felangel/bloc/issues/2526#issuecomment-861633053 answers your question regarding why on<E>
is introduced rather than leaving it as mapEventToState
.
Regarding the expected release date it's still TBD because I'm still trying to get as much feedback as possible from the community. I'll post here with updates as I have them but in the meantime you're welcome to play around with the proposed changes:
dependency_overrides:
bloc:
git:
url: https://github.com/felangel/bloc
path: packages/bloc
ref: 1baa6820629187bbb23a7d799b8c7e9fe73f9a85
flutter_bloc:
git:
url: https://github.com/felangel/bloc
path: packages/flutter_bloc
ref: 1baa6820629187bbb23a7d799b8c7e9fe73f9a85
bloc_test:
git:
url: https://github.com/felangel/bloc
path: packages/bloc_test
ref: 1baa6820629187bbb23a7d799b8c7e9fe73f9a85
Thanks again!
I've put almost all code on the new rails and that looks good! Seen almost 3x code boilerplate decrease with new api and freezed.
Some sample:
@freezed
class DialogState with _$DialogState {
factory DialogState({
required Project project,
}) = _DialogState;
}
@freezed
class DialogEvent with _$DialogEvent {
factory DialogEvent.colorChanged({
required Color color,
}) = ColorChangedEvent;
factory DialogEvent.titleChanged({
required String title,
}) = TitleChangedEvent;
}
class DialogBloc extends Bloc<DialogEvent, DialogState> {
final ProjectsRepository projectsRepository;
final AppRoutingBloc appRoutingBloc;
DialogBloc({
required this.appRoutingBloc,
required this.projectsRepository,
required Project initialProject,
UuidValue? parentId,
}) : super(DialogState(project: initialProject)) {
on<ColorChangedEvent>(colorChanged);
on<TitleChangedEvent>(titleChanged);
}
FutureOr<void> titleChanged(
TitleChangedEvent event, Emitter<DialogState> emit) {
emit(state.copyWith.project(title: event.title));
}
FutureOr<void> colorChanged(
ColorChangedEvent event, Emitter<DialogState> emit) {
emit(state.copyWith.project(color: event.color));
}
}
And still looking for replay_bloc changes... Thanks.
P.S.
For windows-guys: the build_runner can't accept path slushes (path: packages/flutter_bloc
) and you will be unable to use the build_runner.
I've put almost all code on the new rails and that looks good! Seen almost 3x code boilerplate decrease with new api and freezed.
Some sample:
@freezed class DialogState with _$DialogState { factory DialogState({ required Project project, }) = _DialogState; } @freezed class DialogEvent with _$DialogEvent { factory DialogEvent.colorChanged({ required Color color, }) = ColorChangedEvent; factory DialogEvent.titleChanged({ required String title, }) = TitleChangedEvent; } class DialogBloc extends Bloc<DialogEvent, DialogState> { final ProjectsRepository projectsRepository; final AppRoutingBloc appRoutingBloc; DialogBloc({ required this.appRoutingBloc, required this.projectsRepository, required Project initialProject, UuidValue? parentId, }) : super(DialogState(project: initialProject)) { on<ColorChangedEvent>(colorChanged); on<TitleChangedEvent>(titleChanged); } FutureOr<void> titleChanged( TitleChangedEvent event, Emitter<DialogState> emit) { emit(state.copyWith.project(title: event.title)); } FutureOr<void> colorChanged( ColorChangedEvent event, Emitter<DialogState> emit) { emit(state.copyWith.project(color: event.color)); } }
And still looking for replay_bloc changes... Thanks.
P.S. For windows-guys: the build_runner can't accept path slushes (
path: packages/flutter_bloc
) and you will be unable to use the build_runner.
Thatβs awesome, thanks for sharing! Whatβs the issue youβre facing with replay_bloc? I believe it should be compatible as is but Iβll take a closer look shortly.
It still uses mapEventToState
Ah yeah Iβll update it sometime today and let you know π
Hello everyone! π
First of all, I want to thank everyone for the amazing support and community that has grown around the bloc library! ππ
Context
This proposal aims to address 3 problems with the current
mapEventToState
implementation:Predictability
Due to an issue in Dart, it is not always intuitive what the value of
state
will be when dealing with nested async generators which emit multiple states. Even though there are ways to work around this issue, one of the core principles/goals of the bloc library is to be predictable. Therefore, the primary motivation of this proposal is to make the library as safe as possible to use and eliminate any uncertainty when it comes to the order and value of state changes.Learning Curve and Complexity
Writing blocs requires an understanding of
Streams
and async generators. This means developers must understand how to use theasync*
,yield
, andyield*
keywords. While these concepts are covered in the documentation, they are still fairly complex and difficult for newcomers to grasp.Boilerplate
When writing a bloc, developers must override
mapEventToState
and then handle the incoming event(s). Often times this looks something like:The important logic usually lives inside
_mapEventAToState
and_mapEventBToState
andmapEventToState
ends up mainly being setup code to handle determining which mapper to call based on the event type. It would be nice if this could be streamlined.Proposal π₯
I am proposing to remove the
mapEventToState
API in favor ofon<Event>
. This would allow developers to register event handlers by callingon<Event>
whereEvent
is the type of event being handled.on<Event>
would provide a callback(Event event, Emitter<State>) {...}
which would be invoked when an event of typeEvent
is added to the bloc. Developers could thenemit
one or more states in response to the incoming event.For example, if we look at the
CounterBloc
for reference, the current implementation might look something like:With the proposed changes the
CounterBloc
would look something like:If we wanted to support multiple events:
For more complex logic it can be refactored to look like:
These changes address the predictability issues mentioned above because it can be guaranteed that the bloc's state will update immediately when
emit
is called ensuring that cases like this behave as expected:In addition, developers don't have to use async generators (
async*
,yield
,yield*
) which can introduce complexity and undesired behavior in advanced cases.This allows developers to focus on the logic by directly registering an event handler for each type of event which streamlines the bloc code a bit further.
An added benefit is the added consistency across
Cubit
andBloc
-- both trigger state changes viaemit
and the transition fromCubit
toBloc
should become simpler.Becomes
Or as mentioned above (for simple cases)
These changes will obviously be breaking changes that impact blocs (cubits will remain unaffected) so they would all be within the scope of a v8.0.0 release.
The changes would be made in a way that only impacts the bloc
mapEventToState
implementation. The way blocs are used and tested will be 100% backward compatible which means the changes will be scoped to just themapEventToState
code and can ideally be automated via a code mod. There should be no impact to the remaining ecosystem (flutter_bloc
,bloc_test
,hydrated_bloc
,replay_bloc
, etc...).Please give this issue a π if you support the proposal or a π if you're against it. If you disagree with the proposal I would really appreciate it if you could comment with your reasoning.
Thanks so much for all of the continued support and looking forward to hearing everyone's thoughts on the proposal! π
08/31 UPDATE
Hey everyone, just wanted to give a quick update:
We currently have the v8.0.0 branch which replaces
mapEventToState
withon<Event>
; however, we were able to makeon<Event>
backward compatible withmapEventToState
π . You can view the changes as part of the v7.2.0 branch.The current plan is to roll out bloc v7.2.0 in the coming days which will deprecate
mapEventToState
,transformEvents
, andtransformTransitions
and will introduce the newon<Event>
API. We will have a comprehensive migration guide explaining all of the changes and how to migrate over. During this time, we encourage everyone to upgrade to bloc v7.2.0 and start to migrate blocs one at a time. In the meantime, we'll be working on development releases of bloc v8.0.0.As part of v8.0.0 all deprecated APIs from v7.2.0 will be removed and the tentative plan is to publish a stable v8.0.0 release about a month after v7.2.0 has been release. This should give everyone some time to incrementally migrate and for any adjustments to be made. In addition, v7.x.x will still receive bug fixes for the foreseeable future so there should be no pressure/urgency to jump to v8.0.0.
Let us know if you have any questions/concerns.
Thanks for everyone's feedback, patience, and continued support! π π
09/02 UPDATE
We just published bloc v7.2.0-dev.1 which introduces the
on<Event>
API and is backwards compatible which should allow you to migrate incrementally. π β¨Release Notes: https://github.com/felangel/bloc/releases/tag/bloc-v7.2.0-dev.1
Please try it out and let us know if you have any feedback/questions, thanks! π π
09/09 UPDATE
We just published bloc v7.2.0-dev.2 π Release Notes: https://github.com/felangel/bloc/releases/tag/bloc-v7.2.0-dev.2
09/10 UPDATE
We just published bloc v7.2.0-dev.3 π Release Notes: https://github.com/felangel/bloc/releases/tag/bloc-v7.2.0-dev.3
09/21 UPDATE
The time has come π₯ π₯ π₯
bloc v7.2.0 is now out π
π¦ update now: https://pub.dev/packages/bloc/versions/7.2.0 π migration guide: https://bloclibrary.dev/#/migration?id=v720