Bloc: [Proposal?] 'Stream<State> get state' is confusing

[bernaferrari]

I've been developing a relatively large app using Bloc and I think my top 1 issue with Bloc is with state. No, not what you are thinking. I mean, the state variable/getter method.

It has happened to me once, then again, then again, then today. I always cast it, like (currentState as ContrastLoadedState) and I keep trying state before realising my mistake. Just check the screenshot above. It makes no sense for me that currentState returns a State while state returns a Stream<State>.

In BlocBuilder you get a state:
https://github.com/felangel/bloc/blob/805cf865fe0972e7f1bd24f08598d624066434fb/examples/flutter_shopping_cart/lib/catalog/my_catalog.dart#L52

In your own sample you use state = currentState:
https://github.com/felangel/bloc/blob/805cf865fe0972e7f1bd24f08598d624066434fb/examples/flutter_shopping_cart/lib/cart/bloc/cart_bloc.dart#L31

I think you could change state to stateStream and currentState to state or maybe just rename state to stateStream, that way there wouldn't be a state and IntelliJ/VSCode would be smart enough to just suggest currentState.

I'm not sure if this has been a source of bugs/confusion in the past, but it has been a great frustration for me.

[felangel]

Hi @bernaferrari 👋
Thanks for opening an issue!

I'm glad you brought this up and I agree the naming should be improved. What are your thoughts on renaming state to states and leaving currentState as is?

[bernaferrari]

I don't know, I guess it would also be a good change? I personally would probably change it to stateStream or something like this, to keep in track with _stateSubject which already exists, but stateStream is not a perfect name, since it's not obvious what it should be doing from its name, so I guess states would also work fine.

[felangel]

@bernaferrari I've discussed this with the team and we're currently split between:

• bloc.states.listen
• bloc.stream.listen
• bloc.stateStream.listen

What are your thoughts?

[basketball-ico]

@felangel
I prefer bloc.states

Bloc: Takes a [Stream] of [Event]s as input and transforms them into a [Stream] of [State]s as output.

[felangel]

@basketball-ico can you elaborate on why you prefer it over stream? Thanks for the input 🙏

[bigworld12]

stateStream is more intuitive in my opinion, since the name infers the type

[bernaferrari]

I would vote either for states or stateStream. I like the bonus of having the type in the name, but you kind of loose the functionality. It's not that easy to tell what it does. I searched for other usages of the stream<> api and I found someone else also using a variable name in plural when referring to streams. So either way, either of the both are better than state.

Edit: my biggest issue, which made me open this issue, was writability. If tomorrow I need the stateStream (or the states), will it be trivial? Should I know it returns a stream? Is code easy to read? I think I've never used the stateStream method. You are more qualified than me to answer this, read the changes in the PR and think if the naming change makes sense.

[felangel]

@bernaferrari thanks for the additional input. I agree that stateStream is the most descriptive but it's also the most clunky/verbose of the options. In most cases, you would only use the bloc state stream directly if you're subscribing to the stream manually or if you're unit testing the bloc.

Directly Subscribing

// Current
bloc.state.listen((state) { ... });

// Option 1
bloc.states.listen((state) { ... });

// Option 2
bloc.stateStream.listen((state) { ... });

// Option 3
bloc.stream.listen((state) { ... });

Testing

// Current
expectLater(bloc.state, emitsInOrder([ ... ]));

// Option 1
expectLater(bloc.states, emitsInOrder([ ... ]));

// Option 2
expectLater(bloc.stateStream, emitsInOrder([ ... ]));

// Option 3
expectLater(bloc.stream, emitsInOrder([ ... ]));

Personally I think when subscribing it reads a bit weird to have bloc.states.listen and when testing I think it reads weird to have bloc.states, emitsInOrder(...).

The only reason I am advocating for stream is because there should be no other public stream other than the stream of states (by definition of the bloc) so it's redundant to have stateStream.

With all that said, naming is hard and I can be convinced either way haha. Thoughts? @bigworld12 @basketball-ico @bernaferrari

[tenhobi]

I like the bloc.stateStream and bloc.stream. bloc.states might be a bit misleading. I would vote for bloc.stream, because it is short and obvious what it does. 👍

[basketball-ico]

@felangel
I like bloc.states because it is a bloc, you know the output is a Stream, so it's redundant to have bloc.stateStream. (for bloc class)

Sometimes what you're returning is different representations of the same thing, or different kinds of things, or whatever. In that case there does seem to be a convention to choose a plural noun that represents the type of things being returned in the stream.
https://stackoverflow.com/a/28805669

and for me is more easy read

bloc.states.listen((state){...});

than this

bloc.stream.listen((state){...});

thank you :)

[ThinkDigitalSoftware]

I like stream. Because if I were naming variables without types, state would be of type BlocState, states would be of type List<BlocState> and stream would be a Stream out whatever the bloc outputs, which would be Stream<BlocState>. stream also aligns with the StreamController API as a stream of the bloc's output type.
Also, you're not providing multiples states with the stream, you're providing a Stream of type BlocState

[audkar]

Other example which matches this case:

class House {
  final Provider<Window> _window = <...>;
  //0
  Provider<Window> get window => _window;
  //1
  Provider<Window> get windows => _window;
  //2
  Provider<Window> get windowProvider => _window;
  //3
  Provider<Window> get provider => _window;

  final Provider<Door> _door = <...>;
  //0
  Provider<Door> get door => _door;
  //1
  Provider<Door> get doors => _door;
  //2
  Provider<Door> get doorProvider => _door;
  //3
  Provider<Door> get provider2 => _door;
}

OOP uses type of entities for method name which it returns. Not container type in which these entities are wrapped. What will be when class have more methods which return values in same type container? Name them stream2 and stream3?

IMHO 3rd (//3) suggested variant is worst in all cases.

2nd variant (plural form) also sounds strange form me. Stream is a stream. It emits one item at a time.

I personally do not have any problem with current variant (//0) or 3rd one (//3).

[felangel]

@audkar can you clarify? You said you think the 3rd variant is worst in all cases but in your last sentence you say you don’t have a problem with the current variant or the 3rd one.

[audkar]

You said you think the 3rd variant is worst in all cases

brainfarted, sorry. There was proposed three new variants. And 3rd one doesn't looks ok to me.

[felangel]

You said you think the 3rd variant is worst in all cases

brainfarted, sorry. There was proposed three new variants. And 3rd one doesn't looks ok to me.

So you prefer to leave it as is and don’t like bloc.stream?

[audkar]

So you prefer to leave it as is and don’t like bloc.stream

Yes. I would have no idea what this final stream = bloc.stream returns without looking at implementation details or IDE item type.

[rrousselGit]

I by far prefer the current names to all the proposed names.

• stream: that doesn't mean anything. Dart is a typed language. Naming a variable "stream" doesn't bring any extra useful information that the type system doesn't tell you already.

  It's like naming a String "string" or "str". Sure, that's correct, but not helpful at all.

• renaming currentState to state: we're losing meaningful information -> it can change over time.

• state -> states: I don't like it either. It's not a list of state, but a state that changes over time. The plural doesn't mean anything here.
  And having both state and states in the same object makes it difficult to read.

---

Instead, what about removing state entirely?

Instead of:

abstract class Bloc<S> {
  Stream<S> get stream;
  S get currentState;
}

we can have Bloc implement Stream.

abstract class Bloc<S> implements Stream<S> {
  StreamSubscription<S> listen(void listener(S));

  S get currentState;
}

Alternatively, it’d be easier to just expose a “listens” method without implementing Stream and to expose a asStream method

[felangel]

Thanks for the input @rrousselGit.

So you’d prefer:
bloc.listen((state) { ... })

And in tests:
expectLater(bloc, emitsInOrder([ ... ]));

We discussed this briefly within my team and I personally like it but there were concerns about not knowing what you’re listening to. I actually like the way it reads a lot better than the others 👍

[ThinkDigitalSoftware]

I support @rrousselGit's ideas, but I feel that a getter named stream and listening to the bloc provides the same amount of information. At least stream indicates a type while bloc doesn't indicate that its listenable. However, we all got over a small learning curve when learning this library, so we can expect people not to know what to do with something until they learn it. asStream is probably my favorite suggestion. And in the docs, /// returns the output of this bloc as a stream of states.
Easy peasy

[brianegan]

tl;dr: I'd go with state instead of currentState and make the class implement Stream<State>. If ya wanna get crazy, also implement Sink<Event>.

Longer answer:

state vs currentState: I actually made the same mistake as the original author when trying out this library, and don't quite share the same concerns as Remi. I think it'd be a nice change.

When it comes to the name of the stream: In the past, I've created a class very similar to the bloc class, and I actually went with this particular interface (I called mine a Presenter or ReactivePresenter):

class Presenter<State, Action> extends StreamView<State> implements Sink<Action> {

In terms of the Bloc class, it would look like this:

class Bloc<State, Event> extends StreamView<State> implements Sink<Event> {

From one perspective, you could think of the Bloc class in terms of core dart async primitives: It is a class that exposes a Stream<State> and allows you to input values through a Sink<Event>. You also need to dispose of Blocs, and the Sink interface enforces this through the close method and the dart analyzer can warn you if you forget to close Sinks!

In practice, this means you'd work with the bloc class like so:

myBloc.add(IncrementEvent()); // Add events to the Sink instead of dispatching events
myBloc.listen(print); // Print state changes
myBloc.state; // Gets current state
myBloc.close(); // Cleans up the bloc, new name for `dispose`

Dunno if that helps out at all, haha, but I really think it makes sense to think of these classes in terms of those core primitives. Maybe I'm crazy tho :P

[tenhobi]

This is very interesting. I like the idea of implementing it as a Stream and a Sink. :+1:

[ThinkDigitalSoftware]

Definitely doesn't hurt to have a bloc.listen. I think it's clear that you're listening to the state outputs

[felangel]

Thanks @brianegan and great point! I agree that there’s a lot of value in aligning the API with the core primitives. It would be a big breaking change; however, I would be happy to do it if it will benefit the community in the long run. I’ll mess around with the idea and see how it would look/feel. In the meantime I would love to hear what others think about the proposal!

[ThinkDigitalSoftware]

I'm open to it. It would just be different word. add instead of dispatch, bloc.listen instead of bloc.state.listen, right? No functionality changes? I think it would improve adoption of the library

[felangel]

@ThinkDigitalSoftware yup 👍

[brianegan]

As a heads up, changing currentState -> state would need to be a breaking change, but I think implementing the Sink<Event> interface could be done gradually. You could start by implementing the Sink interface methods add and close while keeping and @deprecate dispatch and dispose so folks have time to adjust their own code.

[audkar]

At what state Bloc object would be if client call bloc.sink.close()?

[brianegan]

@audkar It would be the equivalent to calling dispose

[tenhobi]

Also, it would be nice with this change (and something else maybe) make finally a stable release v1.0.0.

[felangel]

@rrousselGit @jorgecoca @bernaferrari thoughts on #572?

[ThinkDigitalSoftware]

What will be done with the stream.map function? Since we have mapEventToState already

[felangel]

@ThinkDigitalSoftware what do you mean? With #572 you'd be able to do bloc.map(...) but as you pointed out in majority of cases you'd probably rely on mapEventToState exclusively.

With the current implementation you can still map on the state stream like bloc.state.map(...).

[ThinkDigitalSoftware]

So we'll have docs on when those will run if it's overridden?

[ThinkDigitalSoftware]

Shouldn't this just implement StreamController? To abstract away the sink?

[felangel]

@ThinkDigitalSoftware we can add that to the docs but it's not specific to the proposed implementation. We were going to implement Sink<Event> in a separate PR to expose add and close in place of dispatch and dispose.

[brianegan]

Implementing StreamController is also an interesting idea, but one reason I didn't suggest that route: it means the Bloc class would need to implement EventSink<Event> instead of Sink<Event>. In practice, that would require the Bloc class implement the addError method in addition to add and close.

IMO, it doesn't make much sense for blocs to have an addError method on them.

[felangel]

Also, if we implemented StreamController<T> wouldn't we be limited by T in terms of what we can add and emit?

[rrousselGit]

Yup. And the bloc instead won't be a stream anymore. We'd need bloc.stream.

[ThinkDigitalSoftware]

Makes sense, however I do believe we already limit what we can add and emit by the current generics on the Bloc class.

[felangel]

@ThinkDigitalSoftware but if you implement StreamController<T> you would only be able to add type T and emit type T which would force your events and states to inherit from some common base class.

[ThinkDigitalSoftware]

I believe map allows you to emit a stream of a different type

Edit: Nevermind. Only if implemented by the user

[felangel]

Implemented by #572, #575 and published in bloc v0.16.0 🎉

Edit: Now included in flutter_bloc v0.22.0 and angular_bloc v0.11.0

[bernaferrari]

Awesome!! Thanks!! Never expected that small issue I had to get such discussion and at the end a very nice solution. ~Now please send us some BMW stickers~