-
-
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
feat!: v5.0.0 #1302
feat!: v5.0.0 #1302
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1302 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 13 +9
Lines 73 134 +61
=========================================
+ Hits 73 134 +61
Continue to review full report at Codecov.
|
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.
🔥🔥🔥
@felangel could you elaborate on the motive for this change (making bloc extend cubit)? Have you already got a clearer idea of when to use cubit vs. bloc? The |
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.
I do not know yet if I like the direction of these changes: IMO, the goal of a constructor should be just to build an instance, but in this case is building the instance plus setting the first/initial value, which I see a bit more confusing than specifying directly initialState
.
The second point to consider is that, unless you are planning to use cubit
for something else, you should be able to accomplish the same goal without cubit
, right?
@jorgecoca that could be said about any class which has fields since you have to initialize them; so basically you're building the instance and setting initial values through the constructor.
Even with the current bloc implementation the initialization happens within the constructor, just routed through a different property.
Dart team also insists on not using properties just for the sake of being fancy when initializing a field, but rather initialize the field directly. I personally like this change, especially in regards to |
@zepfietje I'm working on a formal proposal/roadmap for cubit and will share it as soon as I can. @jorgecoca as @RollyPeres mentioned, I personally think the proposed change actually makes more sense because the |
@jorgecoca @RollyPeres @chimon2000 can you take another look when you have a min and lmk if you have any additional feedback that I haven't incorporated? Based on the discussions in #1304 it seems safe to proceed. Thanks 👍 |
6fab62a
to
adedd5e
Compare
7d3abe9
to
b617e4f
Compare
I think this is a good change. Great work @felangel !!! Thank you !! Lots of Thumbs Up!! |
Status
READY
Breaking Changes
YES
Description
Stream
. (non-breaking)null
states (closes It is not possible for state to be null #1312)initialState
override in favor of providing the initial state viasuper
. (closes [BREAKING][Proposal] remove initialState override #1304)would become
The other consequence of this,
initialState
will no longer exist as a public getter on blocs so I would love to hear what everyone thinks about this change. If the community prefers to keep theinitialState
signature as is for now, I'm definitely open to leaving it as is but feel it's a bit clunky and could be streamlined.BlocSupervisor
and renameBlocDelegate
toBlocObserver
would become:
And initialization would go from:
to
EventSink
rather thanSink
which exposesaddError
for a more intuitive, friendly API to handle exceptions within a bloc.Currently you have to
rethrow
:Or
throw
when usingEither
:It is not immediately clear/obvious that (re)throwing bubbles the exception up to
onError
and can be improved:Todos
Impact to Remaining Code Base