-
-
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
[BREAKING][Proposal] remove initialState override #1304
Comments
LGTM. I don't see the modification as an issue. But less boilerplate is fine with me! |
Would like to see the major changes. It should be notified to all the users that the |
Hi @felangel 👋 Thanks for opening an issue! 😂 |
The proposed syntax is, probably, the most intuitive. I saw many newbies' question about how to initiate the state thinking in this proposal way. Respective to the breaking change, refactoring won't be a big extra effort. Then I agree with the proposal. |
Looks good. Less boilerplate is 🔥 |
You the man. I completely trust your judgement. But does this mean I could pass in an instance of, say, a user object into the initial state? That could be really handy. |
Eventhough it be a huge change for my apps I believe that this change makes the code easier to understand and also as @nhwilly said it could be very handy to be able to pass instances to a bloc. Nice thinking! |
Like the approach but breaking changes will result in the overhead, but I think it will be a good tradeoff |
Related: felangel/cubit#6 I think there's some value in that this allows teams to scale capabilities without drastically changing underlying dependencies. If you're already using cubit, and want the abilities of bloc for a feature, the change is minimal. |
Sounds good and at first glance i don't see any backdraws. I sometime use a constructor like this:
But that could easily be changed to a ternary operator in the super call. |
The third possible way is using the new class Bloc ... {
late final initialState;
} But this brings more responsibility to user to actually set the initial state before using it. I would personally go for the way with a constructor. Maybe even use it as named parameter? |
I think that removing However, personally, I don't see a problem with the current state of things because:
My reasoning in point 2 is the following. Instead of writing this... MyBloc(
initialState: InitialState(),
); ... one could make this ... final state = InitialState(),
MyBloc(
initialState: state,
); ... and the Someone using the BLoC must know that the initial state doesn't have to be modified after the instantiation of the bloc. With this knowledge, both the constructor way and the getter way are fine in my opinion because the user has the responsibility to not touch the internals. If any state had a There will also be the need for a refactor of course and I don't think it would take too long, but still it has to be taken into account |
LGTM, I don't remember having to change the value of the initialState. |
Well i feel that initialstate getter clearly defines the purpose, the new super declaration appears more compact. Well, less code is better, right? |
Can we have a named argument in super()? |
In Felix we trust 👍❤️ |
I don't know if I'm misusing the bloc pattern, but I use the initialState when initializing a custom Navigator like so: return BlocProvider<CoreBloc>.value(
value: coreBloc,
child: Navigator(
key: coreBloc.router.navigator,
// initial state used here
initialRoute: coreBloc.router.stageToName[coreBloc.initialState.stage],
onGenerateRoute: coreBloc.router.onGenerateRoute,
),
); Though I guess I could instantiate the |
@VKBobyr you could always expose your own public |
@felangel Yeah, decoupling router and bloc would certainly solve the issue and is probably a better design choice anyway. I do support the change then! |
Definitely is an issue if it causes a leak. Thanks for the fix, Felix! |
It looks like having a required, named parameter isn't ideal either because there won't be any warning/error if you forget to add it until NNBD (related issue: dart-lang/sdk#42438). For example, class CounterBloc extends Bloc<CounterEvent, int> {
// Code will compile without the following line
CounterBloc() : super(initialState: 0);
@override
Stream<int> mapEventToState(CounterEvent event) async* {}
} The above snippet will compile with no warnings/errors and will result in the bloc having a This will be resolved once NNBD lands in stable but until then I think it will be safer to use a non-named parameter like: class CounterBloc extends Bloc<CounterEvent, int> {
// Code will not compile without the following line
CounterBloc() : super(0);
@override
Stream<int> mapEventToState(CounterEvent event) async* {}
} This will ensure that an initial state is always specified (even if it's |
@felangel what about using |
I think the |
Having On the other hand, if Given the great documentation for this package, it isn't hard for a developer to figure out how to set the initial state. |
Would be great if there are linters to have similar way as
If the developer explicitly put null there so that he knows what he/she put in the constructor.
The documentation is great. If people enforce to have naming conventions like
In my earlier comment, I did not specify named parameters to be the solution. when i said I think, I am just suggesting ideas. But thanks for justifying it. |
I am struggling with this update. How can I call super when there is logic involved in setting the initialState? e.g. class MyBloc extends Bloc< MyEvent, MyState > {
OtherBloc otherBloc;
MyBloc({@required this.otherBloc});
@override
MyState get initialState {
final state = otherBloc.state;
if (state is OtherStateFoo) {
return MyStateFoo;
} else {
return MyStateBar;
}
}
} Ugly solution 1: Ugly solution 2: class MyBloc extends Bloc< MyEvent, MyState > {
OtherBloc otherBloc;
MyBloc({@required this.otherBloc}): super(initialState(otherBloc));
static MyState initialState (OtherBloc staticOtherBloc){
final state = staticOtherBloc.state;
if (state is OtherStateFoo) {
return MyStateFoo;
} else {
return MyStateBar;
}
}
} Any other suggestions? |
@TomEversdijk you can do: import 'package:bloc/bloc.dart';
import 'package:meta/meta.dart';
abstract class MyOtherEvent {}
abstract class MyOtherState {}
class MyOtherStateA extends MyOtherState {}
class MyOtherStateB extends MyOtherState {}
class MyOtherBloc extends Bloc<MyOtherEvent, MyOtherState> {
MyOtherBloc() : super(MyOtherStateA());
@override
Stream<MyOtherState> mapEventToState(MyOtherEvent event) async* {}
}
abstract class MyEvent {}
abstract class MyState {}
class MyStateA extends MyState {}
class MyStateB extends MyState {}
class MyBloc extends Bloc<MyEvent, MyState> {
final MyOtherBloc myOtherBloc;
MyBloc({@required this.myOtherBloc})
: super(myOtherBloc.state is MyOtherStateA ? MyStateA() : MyStateB());
@override
Stream<MyState> mapEventToState(MyEvent event) async* {}
} Hope that helps 👍 |
Is your feature request related to a problem? Please describe.
As a developer, having to override
initialState
when creating a bloc presents two main issues:initialState
of the bloc can be dynamic and can also be referenced at a later point in time (even outside of the bloc itself). In some ways, this can be viewed as leaking internal bloc information to the UI layer.Describe the solution you'd like
Rather than defining the initial state like:
I am proposing:
This will avoid exposing
initialState
publicly and also reduces the amount of code needed especially considering in most cases a bloc will already require a constructor for injecting a repository like:The above case could be simplified to:
Additionally, it would no longer be possible to dynamically modify the value of a bloc's
initialState
after it has been initialized.In the current state, the following code is totally valid but could potentially have many unintended side-effects
Additional context
This would also enable reduction in boilerplate for
HydratedBloc
Before:
We can simplify the API and eliminate the need to explicitly call
super.initialState
Would love to hear everyone's feedback on this especially if you are specifically relying on
initialState
for something outside of just seeding the bloc's initial state 👍Looking forward to hearing everyone's thoughts 😄
UPDATE
After going through a lot of feedback it seems a good alternative would be to use a named parameter in
super
.The text was updated successfully, but these errors were encountered: