-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
[Proposal] Bloc and Cubit to extend BlocBase #2235
Comments
Well.. I like the idea of greatly simplifying the API and I like the reminder that we are working with streams when we are working with Bloc. Seems like a pretty elegant solution ! Are there any downsides, besides a refactor? |
In my opinion, the main downside is the breaking change for the rename + the introduction of a |
I think this is a great approach. From the looks of it, I don't see any downsides besides a small refactor. LGTM overall. |
I like the idea 👍 and have a few comments:
|
To me I'd propose keeping
I don't believe so.
Why? Cubit can expose the state stream the same way that |
@wujek-srujek thanks for the feedback!
Yeah that works. I'm not good with names either haha.
Yeah good call we can rename for consistency 👍
Yeah I went back and forth on this as well. Cubits still technically have transitions but they just don't include event information. We could remove Event from the |
@Jomik thanks for the feedback! Just to clarify you're proposing the following:
We would still need to have a concept of Thanks for the feedback! I really appreciate it 💯 |
Correct.
I honestly do not think it matters so much. I'd much rather keep the name
I did not think of it, but like this! It allows for some nice default stringification for both cases, if we just need to log our transitions. |
We could also use a more neutral name like: |
Just some prototyping, I'm not sure if this generics-galore is worth it at all, bit it is possible to define class Transition<State> {
final State previous;
final State next;
const Transition(this.previous, this.next);
}
class BlocTransition<Event, State> extends Transition<State> {
final Event event;
const BlocTransition(State previous, State next, this.event): super(previous, next);
}
abstract class StateStream<State, T extends Transition<State>> {
void onTransition(T transition);
}
class Bloc<Event, State> extends StateStream<State, BlocTransition<Event, State>> {
@override
void onTransition(BlocTransition<Event, State> transition) {}
}
class Cubit<State> extends StateStream<State, Transition<State>> {
@override
void onTransition(Transition<State> transition) {}
} I personally rally don't like the idea of |
@wujek-srujek in v6.0.0 |
Just a thought on the naming. |
@fabriziocacicia @wujek-srujek another option that @Jomik brought up is |
|
the traditional bloc works fine for me unless there is some added benefit I am not seeing from this proposal. |
@gondaimgano thanks for the feedback! To be clear, this proposal is to simplify the existing public API surface area of bloc and cubit and to make the relationship between bloc and cubit more natural. As a developer, you will still be able to use bloc and cubit as you normally do with a few minor breaking changes (mentioned above). |
I hate to be the one to say it, but this looks more and more like a |
Guys, BlocStream is a pretty good name. What else could it be? BlocCurrent? BlocFlow? BlocPower? BlocWire? BlocWave? BlocVacuum? BlocRiver xD? |
BlocBridge, BlocTx, BlocPipe? |
I think BlocStream is pretty good name :D |
@Gene-Dana Understood ;d |
Done as part of #2234 🎉 |
Summary
BlocBase
rather than having bloc directly extend cubit or vice versa.Please leave a 👍 if you agree with the proposal or a 👎 if you disagree. If you disagree, it would be great if you could also leave a comment explaining why with any alternative suggestions 🙏
Breaking Changes
BlocObserver
Before
After
Bloc and Cubit
Stream
andSink
(Close instances ofdart.core.Sink
. warning by IDE #1942, [Discussion] Should Cubit expose Stream API? #1429)Stream<State>
getter insteadBefore
After
Note:
listen
will still be available as part of both the bloc and cubit public api until v8.0.0Problem
As a developer, the relationship between blocs and cubits is a bit awkward. When cubit was first introduced it began as the base class for blocs which made sense because it had a subset of the functionality and blocs would just extend
Cubit
and define additional APIs. This came with a few drawbacks:cubit
for accuracy or they would need to be kept asbloc
for consistency even though hierarchically it is inaccurate (Should certain types be renamed?BlocXXX
->CubitXXX
#1708, [Discussion] Add Cubit Flutter Widgets? #1560).Stream
and implementEventSink
in order to have a common base which widgets likeBlocBuilder
,BlocListener
, etc. can be implemented against ([Discussion] Should Cubit expose Stream API? #1429).Later, we experimented with inverting the relationship and making bloc the base class which partially resolved the first bullet above but introduced other issues:
mapEventToState
,add
, etc. (Migration notes missing for 7.0 #2228)Proposal
One way to address these issues is to introduce a base class for both
Bloc
andCubit
so that upstream components likeBlocBuilder
,BlocProvider
, etc. can interoperate with both bloc and cubit instances.BlocBase Public API
An interface for the core functionality implemented by both Bloc and Cubit.
This would allow for both bloc and cubit to implement this
BlocBase
API and still maintain specialized public APIs (mapEventToState
,add
, etc.)This change would also allow us to expose a getter to access the
Stream<State>
as opposed to having bloc and cubit implementStream<State>
which would address #1942 & #1429 and greatly simplify the public API of both bloc and cubit.The text was updated successfully, but these errors were encountered: