-
-
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
[Discussion] Should Cubit expose Stream API? #1429
Comments
I agree, in general, users don’t need to know too much. When advanced features are needed, the .stream method is also very good |
I feel we've come full circle if we remove it again. That being said, I'm very much in favour of clean APIs.
Personally, we've never used the Stream API present on Bloc. The big question is if anyone else do. |
Not sure if it is a relevant point but one of the things that are unclear to me is how am I supposed to combine bloc with the FutureBuilder widget. A workaround which I came up with is to have a future as a prop in my state. However this does not seem very elegant. I suppose exposing the steam directly from bloc would help with that. |
@larssn yeah I totally agree feels like we're going back in time haha. The idea of extending @fotiDim can you give an example of when you use a |
@felangel I am using bloc to manage fetching a bunch of POIs from an API and I use MapBox to draw them on a map as markers. Mapbox has an |
@fotiDim you can use cubit for this like so: class MyCubit extends Cubit<MyState> {
MyCubit() : super(MyState.initial());
Future<void> loadAssets() async {
emit(MyState.loading());
await Future.wait([
_future1,
_future2,
]);
emit(MyState.success());
}
} Then, you can use |
We've argued about this previously, but this discussion is even more reasons to merge With this change, both objects would be basically identical, and I do not believe that we cannot reconcile the small details around it. The current situation both duplicate the effort in doc/tools, split/confuse the community, My feelings put aside, Even without mentioning auto-complete, there's no way implementing So I think this is a good change. |
@rrousselGit the change for bloc (and consequently cubit) to extend
Can you please clarify what you were thinking? I would love to better understand what your preferred direction would be for each library. Are you suggesting cubit is merged into state_notifier or vice-versa, how would that affect the current implementation of bloc which extends cubit as well as the remainder of the ecosystem (bloc_test, hydrated_bloc, replay_bloc, etc...)?
The bloc library has been around just as long and the addition of In any case, I'm not here to try to prove that one approach is better than the other but rather to do what is best for the community. |
It's funny for someone as smart as you are to act like this with any chance you get. You know there are other smart people as well in this community doing their own work! It's not all about they doing things that should satisfy you. Other people have ideas too you know ? Surely you must realize you're not the only one having good ideas, right ? It's completely unfair and childish for you to keep trying to force yourself and your own work upon everyone with this "I'm a victim" attitudine you embrace. You're the one taking everything as a competition I feel, everyone else is having fun building cool stuff for community. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The
I wouldn't mind if you had good reason/intentions to do so -- I really don't want this to be a competition. The bloc library exists because it makes my life easier and I thought others might benefit from it as well. |
Let's continue this discussion in a separate issue – it's my bad for talking about merging packages in this issue. I do agree with the change proposed here, which is no-longer implementing |
No worries! Regarding the original topic, I have a few follow-up questions which I would love to get people's thoughts on:
cubit.listen((state) {...}); Or would you instead prefer to have to cubit.stream.listen((state) {...});
My understanding was @chimon2000 didn't like the idea of the Thanks! |
This indirectly answers the question about:
If we circle back to the comparison with This listening mechanism does not rely on the event-loop and is implemented using a simple This both consumes less memory and is significantly faster than From there, |
Just in case participants on this issue have forgotten that we are an extension of Flutter's code of conduct, I'll repost this here.
https://github.com/flutter/flutter/blob/master/CODE_OF_CONDUCT.md We should not resort to name-calling when someone voices their feelings. What we should strive to do is acknowledge them and try to empathize. |
As someone with no allegiance to either bloc or state_notifier/provider, I would say that
On that second point, I actually had thought about that originally when I suggested Cubit be merged into Bloc, but I assumed that would be an idea to iterate on. If I'm being 100% transparent I'm not a huge fan of Dart's Stream implementation, but that's another can of worms. I like the idea of exploring this conversation in another issue - maybe a formal RFC. I greatly appreciate the hard work of @felangel and @rrousselGit, and all the contributors to these projects. I think it makes sense to explore deduplication and reduce complexity wherever possible, so there's nothing wrong with asking if it is possible. |
So about the Stream API, I wouldn't mind access to the underlying Stream(Controller) in Cubit, since I want to be able to transform it's emissions. However I don't necessarily need Cubit to inherit from Stream to accomplish this since it muddles the simplicity of the API. I wanna move away from the normal event-driven flow since I'm interested in less indirection (which events cause), and less boilerplate (which Blocs generate a metric ton of). I like the direct nature of method calls, plus it's easier to teach new people. While we're at the discussion of streams: Sorry for the critique, I do consider this one of the best documented libraries I've ever used. The barrier for entry is basically zero and everyone should be able to understand and implement the BLoC pattern using it. But I'm torn about needing it. |
@larssn thanks so much for the feedback!
Can you please elaborate on this? The idea behind In my opinion using a
As always, I really appreciate your feedback and don't be sorry! Criticism is how we improve 😄 |
@felangel You're right about snapshots and initialData; it is a bit of a pain point with the plain StreamBuilder implementation. The thing thats harder with Cubit, as I mentioned, is transforming the outgoing stream inside the cubit class. It's something I personally require quite often. May I suggest moving Cubit closer to the vanilla way of working with Streams, but removing the pain points you listed.
In my experience, it can be problematic to make things TOO simple, as it usually means flexibility will suffer. There's usually a balance, and I think giving access to the underlying Stream is a must, but I would recommend using composition instead of inheritance. Anyway, enough of my ramblings. Thanks for listening 🙂 |
What does this mean out of curiosity? |
@rrousselGit Though I suppose I can technically |
I left a message here too. You are both experts. |
@felangel I understand what you suggest however I cannot do the same thing with |
IMHO, just expose the Stream<State> get stream => (_controller ??= StreamController<State>.broadcast()).stream; Instead of: StreamSubscription<State> listen(
void Function(State) onData, {
Function onError,
void Function() onDone,
bool cancelOnError,
}) {
_controller ??= StreamController<State>.broadcast();
return _controller.stream.listen(
onData,
onError: onError,
onDone: onDone,
cancelOnError: cancelOnError,
);
} |
@jifalops thanks for the input. The reason for the current implementation is to make blocs and cubits extend |
|
Done as part of #2234 🎉 |
The text was updated successfully, but these errors were encountered: