Skip to content
This repository has been archived by the owner on Jul 9, 2020. It is now read-only.

[DISCUSSION] Should Cubit expose Stream API? #46

Closed
chimon2000 opened this issue Jul 3, 2020 · 12 comments
Closed

[DISCUSSION] Should Cubit expose Stream API? #46

chimon2000 opened this issue Jul 3, 2020 · 12 comments
Assignees
Labels
feedback wanted Looking for feedback from the community question Further information is requested
Projects

Comments

@chimon2000
Copy link

Something I immediately noticed is that because Cubit inherits from Stream, it surfaces the entire Stream API. This seems like a leaky abstraction, and I'm wondering whether there is some way that the API could be hidden?

One option I could see is for Cubit to have a protected/private CubitStream<State> stream property, rather than inherit from it. The most minimal change might be something like the following.

abstract class Cubit<State> {
  @protected
  CubitStream<State> stream;

  State get state => _stream.state;
}
@felangel
Copy link
Owner

felangel commented Jul 3, 2020

Hi @chimon2000 👋
Thanks for opening an issue!

Can you please elaborate a bit on why you feel surfacing the Stream API is undesirable? In my opinion, this makes cubit (and bloc) a lot more versatile and powerful. Thanks 😄

@felangel felangel self-assigned this Jul 3, 2020
@felangel felangel added question Further information is requested waiting for response Waiting for follow up labels Jul 3, 2020
@felangel felangel added this to To do in cubit via automation Jul 3, 2020
@chimon2000
Copy link
Author

I agree it makes it more powerful, but I also think the trade-off is added complexity most users may not care about. I don't think that the developer should need to know anything about Streams (which they don't) to use Cubit, however when you're intellisense presents you with the entire Stream API it might appear daunting. If the stream is a protected variable, that may be a way to convey to the user: "only use this if you really know what you're doing."

I also acknowledge that this is subjective and influenced by my initial reaction to seeing everything but state & init in my IDE. 😊

image

@felangel
Copy link
Owner

felangel commented Jul 3, 2020

Thanks for clarifying! I see what you're saying and would love to hear what other members of the community think.

If most individuals would rather not have the Stream API at the surface level of Cubit we can easily move it one level lower but it would technically be a breaking change and it would not be consistent with bloc which also extends Stream. Check out felangel/bloc#558 (comment) for more context around why we decided to implement Stream in the first place.

Thoughts?

@felangel felangel added the feedback wanted Looking for feedback from the community label Jul 3, 2020
@chimon2000
Copy link
Author

Definitely should surface this one to the larger community. I guess I should have opened this issue before I suggested integrating bloc & cubit 😅

@felangel felangel changed the title Only surface Cubit api [DISCUSSION] Should Cubit expose Stream API? Jul 3, 2020
@felangel felangel pinned this issue Jul 3, 2020
@felangel
Copy link
Owner

felangel commented Jul 3, 2020

Haha no worries! I'll think about this some more and wait to hear what the rest of the community thinks 👍

One thing to note is you typically shouldn't need to use this within a cubit so in most cases you shouldn't bump into the Stream API unless you are looking for it. To me it doesn't feel like this is adding a lot of additional complexity but then again I'm biased 😛

@chimon2000
Copy link
Author

Yeah, I was actually just using this for simulation purposes, I'm pretty sure i discovered this by accident while emitting a change.

@narcodico
Copy link

To me it looks more like a discussion about whether or not CubitStream<State> should implement StreamController<State>.

But that's not feasible because you won't be able to extend bloc from CubitStream<State> and also implement EventSink<Event>, the reason being that you cannot implement both EventSink<Event> and EventSink<State> due to difference in types.

If we'd to adhere to dart primitives design then having a stream property on Cubit<State> would imply that Cubit<State> or CubitStream<State> is a StreamController, which cannot be the case for the aforementioned reason.

@felangel felangel removed the waiting for response Waiting for follow up label Jul 4, 2020
@samandmoore
Copy link

I think that ideally, I'd want Cubit/Bloc to present a limited API that corresponded specifically to what makes them special when compared to a Stream. Then, I'd want there to be an asStream() method that would expose the raw stream to me if I found myself needing it. In the last 6 months of working with Bloc I've only dealt with a Bloc as a Stream once, so I'm not sure that a Bloc presenting as a Stream by default is particularly useful to me. Additionally, I've definitely had a few conversations with folks that are new to Bloc / Flutter and they've been confused by all the methods they can call on a Bloc. I suspect that with Cubit that will be even more common for newcomers because it's more method-oriented than Bloc.

I'm not sure how much it matters / how valid this is, but another potential thing to consider is that if Bloc/Cubit is a Stream, it can be plugged directly into places that work with Streams, like flutter_hooks and river_pod. Although, in that case, it's probably better to expose a custom hook / provider than to just use the Stream* ones. So, perhaps this doesn't matter that much after all haha.

I don't feel that strongly about this, but that's my 2 cents.

@narcodico
Copy link

@samandmoore asStream is mainly used to convert Futures to Streams. But as you already mentioned, bloc is already a Stream. But I do feel like a clean API for cubit would be beneficial(especially to newcomers) since it's method based, but having the stream getter would kinda go against the dart primitives design. Trade-offs. Tough one.🤦‍♂️

@samandmoore
Copy link

Hm. That's surprising to me that it would go against the dart primitives design. To me, it seems like easily being able to convert a value into a related but different value, e.g. a future to a stream, or in our case, a cubit/Bloc to a stream, would be a good feature of this specific primitive.

@samandmoore
Copy link

Definitely trade offs here though 🤔

@felangel
Copy link
Owner

felangel commented Jul 9, 2020

moved to felangel/bloc#1429

@felangel felangel closed this as completed Jul 9, 2020
cubit automation moved this from To do to Done Jul 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feedback wanted Looking for feedback from the community question Further information is requested
Projects
No open projects
cubit
  
Done
Development

No branches or pull requests

4 participants