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
Remove RxDart dependency #839
Conversation
onData, | ||
onError: onError, | ||
onDone: onDone, | ||
cancelOnError: cancelOnError, | ||
); | ||
} | ||
|
||
// Creates a Stream that emits the current state and any future states | ||
Stream<State> _createStateStream() async* { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that came up for me: How important is it that blocs emit the latest event when you first listen? Is it even necessary?
I know that'd be a bigger breaking change to discuss, but seems like most blocs are hooked up to StreamBuilder
widgets. Since you already have the initialState
to pass to the StreamBuilder
for the initial build, is it really necessary for this class to emit the latest value to all listeners (triggering 2 builds for each BlocBuilder widget?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha I was thinking about that yesterday as well. I think keeping the behavior as is for now makes sense. I need to think through what the implications of removing that are because actually BlocBuilder and BlocListener already use skip(1) to avoid the 2 builds issue.
@@ -157,15 +165,15 @@ abstract class Bloc<Event, State> extends Stream<State> implements Sink<Event> { | |||
/// ``` | |||
Stream<State> transformStates(Stream<State> states) => states; | |||
|
|||
void _bindStateSubject() { | |||
void _bindEventsToStates() { | |||
Event currentEvent; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment: It feels like this could lead to bugs where asyncExpand
is not used in transformEvents
, and therefore events will be delivered to the forEach
that might respond to different events.
It might be better to capture the event + states together and deliver those to the forEach
together
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah good point. Haven't run into that case so far but I see what you're saying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thoughts on #838?
Closing this since #821 was merged. Thanks so much for your help as always! 🙏 |
Status
READY/IN DEVELOPMENT/HOLD
Breaking Changes
YES | NO
Description
A few sentences describing the overall goals of the pull request's commits.
Related PRs
List related PRs against other branches:
Todos
Steps to Test or Reproduce
Outline the steps to test or reproduce the PR here.
Impact to Remaining Code Base
This PR will affect: