-
-
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?] 'Stream<State> get state' is confusing #558
Comments
Hi @bernaferrari 👋 I'm glad you brought this up and I agree the naming should be improved. What are your thoughts on renaming |
I don't know, I guess it would also be a good change? I personally would probably change it to |
@bernaferrari I've discussed this with the team and we're currently split between:
What are your thoughts? |
@felangel
|
@basketball-ico can you elaborate on why you prefer it over |
|
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. |
@bernaferrari thanks for the additional input. I agree that 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 The only reason I am advocating for With all that said, naming is hard and I can be convinced either way haha. Thoughts? @bigworld12 @basketball-ico @bernaferrari |
I like the |
@felangel
and for me is more easy read
than this
thank you :) |
I like |
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 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 ( |
@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. |
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? |
Yes. I would have no idea what this |
I by far prefer the current names to all the proposed names.
Instead, what about removing Instead of: abstract class Bloc<S> {
Stream<S> get stream;
S get currentState;
} we can have 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 |
Thanks for the input @rrousselGit. So you’d prefer: And in tests: 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 👍 |
I support @rrousselGit's ideas, but I feel that a getter named |
tl;dr: I'd go with Longer answer:
When it comes to the name of the stream: In the past, I've created a class very similar to the
In terms of the Bloc class, it would look like this:
From one perspective, you could think of the 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 |
@ThinkDigitalSoftware yup 👍 |
As a heads up, changing |
At what state Bloc object would be if client call |
Also, it would be nice with this change (and something else maybe) make finally a stable release v1.0.0. |
@rrousselGit @jorgecoca @bernaferrari thoughts on #572? |
What will be done with the stream.map function? Since we have mapEventToState already |
@ThinkDigitalSoftware what do you mean? With #572 you'd be able to do With the current implementation you can still map on the state stream like |
So we'll have docs on when those will run if it's overridden? |
Shouldn't this just implement StreamController? To abstract away the sink? |
@ThinkDigitalSoftware we can add that to the docs but it's not specific to the proposed implementation. We were going to implement |
Implementing StreamController is also an interesting idea, but one reason I didn't suggest that route: it means the IMO, it doesn't make much sense for blocs to have an |
Also, if we implemented |
Yup. And the bloc instead won't be a stream anymore. We'd need |
Makes sense, however I do believe we already limit what we can add and emit by the current generics on the Bloc class. |
@ThinkDigitalSoftware but if you implement |
I believe map allows you to emit a stream of a different type
Edit: Nevermind. Only if implemented by the user
|
Awesome!! Thanks!! Never expected that small issue I had to get such discussion and at the end a very nice solution. |
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.
![image](https://user-images.githubusercontent.com/351125/66448646-93c0fa80-ea28-11e9-8e9c-f65af5716f77.png)
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 thatcurrentState
returns aState
whilestate
returns aStream<State>
.In BlocBuilder you get a state:
bloc/examples/flutter_shopping_cart/lib/catalog/my_catalog.dart
Line 52 in 805cf86
In your own sample you use state = currentState:
bloc/examples/flutter_shopping_cart/lib/cart/bloc/cart_bloc.dart
Line 31 in 805cf86
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.
The text was updated successfully, but these errors were encountered: