[go: nahoru, domu]
Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat!: v5.0.0 #1302

Merged
merged 56 commits into from
Jul 2, 2020
Merged

feat!: v5.0.0 #1302

merged 56 commits into from
Jul 2, 2020

Conversation

felangel
Copy link
Owner
@felangel felangel commented Jun 13, 2020 •
edited
Loading

Status

READY

  • bloc v5.0.0-dev.11
  • flutter_bloc v5.0.0-dev.5
  • angular_bloc v6.0.0-dev.4
  • bloc_test v6.0.0-dev.4

Breaking Changes

YES

Description

class CounterBloc extends Bloc<CounterEvent, int> {
  @override
  int get initialState => 0;
}

would become

class CounterBloc extends Bloc<CounterEvent, int> {
  CounterBloc() : super(0);
}

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 the initialState 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.

  • BREAKING: remove BlocSupervisor and rename BlocDelegate to BlocObserver
class SimpleBlocDelegate extends BlocDelegate {
 @override
  void onEvent(Bloc bloc, Object event) {
    print('bloc: ${bloc.runtimeType}, event: $event');
    super.onEvent(bloc, event);
  }

  @override
  void onTransition(Bloc bloc, Transition transition) {
    print('bloc: ${bloc.runtimeType}, transition: $transition');
    super.onTransition(bloc, transition);
  }

  @override
  void onError(Bloc bloc, Object error, StackTrace stackTrace) {
    print('bloc: ${bloc.runtimeType}, error: $error');
    super.onError(bloc, error, stackTrace);
  }
}

would become:

class SimpleBlocObserver extends BlocObserver {
  @override
  void onEvent(Bloc bloc, Object event) {
    print('bloc: ${bloc.runtimeType}, event: $event');
    super.onEvent(bloc, event);
  }

  @override
  void onTransition(Bloc bloc, Transition transition) {
    print('bloc: ${bloc.runtimeType}, transition: $transition');
    super.onTransition(bloc, transition);
  }

  @override
  void onError(Bloc bloc, Object error, StackTrace stackTrace) {
    print('bloc: ${bloc.runtimeType}, error: $error');
    super.onError(bloc, error, stackTrace);
  }
}

And initialization would go from:

BlocSupervisor.delegate = SimpleBlocDelegate();

to

Bloc.observer = SimpleBlocObserver();
  • Bloc to implement EventSink rather than Sink which exposes addError for a more intuitive, friendly API to handle exceptions within a bloc.

Currently you have to rethrow:

Stream<MyState> mapEventToState(MyEvent event) async* {
  try {
    await someOperation();
  } on SomeSpecificException catch (_) {
    yield SomeSpecificFailureState();
    rethrow; // currently necessary to propagate error to `onError` in `BlocObserver`.
  }
}

Or throw when using Either:

yield* either.fold(
  (failure) async * {
    yield FailureState(failure);
    throw failure;
  }, 
  (data) => ...
)

It is not immediately clear/obvious that (re)throwing bubbles the exception up to onError and can be improved:

Stream<MyState> mapEventToState(MyEvent event) async* {
  try {
    await someOperation();
  } on SomeSpecificException catch (error, stackTrace) {
    yield SomeSpecificFailureState();
    addError(error, stackTrace);
  }
}
yield* either.fold(
  (failure) async * {
    yield FailureState(failure);
    addError(failure);
  }, 
  (data) => ...
)

Todos

  • Tests
  • Documentation
  • Examples

Impact to Remaining Code Base

  • Breaking Changes introduced in bloc v5.0.0

@felangel felangel self-assigned this Jun 13, 2020
@felangel felangel requested a review from jorgecoca June 13, 2020 19:30
@felangel felangel added enhancement candidate Candidate for enhancement but additional research is needed feedback wanted Looking for feedback from the community pkg:bloc This issue is related to the bloc package labels Jun 13, 2020
@felangel felangel added this to In progress in bloc via automation Jun 13, 2020
@felangel felangel mentioned this pull request Jun 13, 2020
Closed
@codecov
Copy link
codecov bot commented Jun 13, 2020 •
edited
Loading

Codecov Report

Merging #1302 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1302   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4        13    +9     
  Lines           73       134   +61     
=========================================
+ Hits            73       134   +61
Impacted Files Coverage Δ
packages/flutter_bloc/lib/src/bloc_listener.dart 100.00% <ø> (ø)
...lutter_bloc/lib/src/multi_repository_provider.dart 100.00% <ø> (ø)
...ages/flutter_bloc/lib/src/repository_provider.dart 100.00% <ø> (ø)
packages/bloc/lib/src/bloc.dart 100.00% <100.00%> (ø)
packages/bloc/lib/src/bloc_observer.dart 100.00% <100.00%> (ø)
packages/bloc/lib/src/transition.dart 100.00% <100.00%> (ø)
packages/flutter_bloc/lib/src/bloc_builder.dart 100.00% <100.00%> (ø)
packages/flutter_bloc/lib/src/bloc_consumer.dart 100.00% <100.00%> (ø)
packages/flutter_bloc/lib/src/bloc_provider.dart 100.00% <100.00%> (ø)
...ages/flutter_bloc/lib/src/multi_bloc_listener.dart 100.00% <100.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a7b7b1...34803fa. Read the comment docs.

chimon2000
chimon2000 previously approved these changes Jun 13, 2020
View reviewed changes
Copy link
@chimon2000 chimon2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥🔥🔥

@zepfietje
Copy link
Contributor

@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 initialState change does seem clean to me. 👌

jorgecoca
jorgecoca reviewed Jun 14, 2020
View reviewed changes
Copy link
Collaborator
@jorgecoca jorgecoca left a 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?

@narcodico
Copy link
Contributor

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.

@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.

class Model {
  Model._(): field = '';
  const Model(this.field);

  final String field;
}

Even with the current bloc implementation the initialization happens within the constructor, just routed through a different property.

 Bloc() {
    _state = initialState;
    _bindEventsToStates();
  }

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 HydratedBloc.

@felangel
Copy link
Owner Author
felangel commented Jun 15, 2020 •
edited
Loading

@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 initialState should not be something that can change after the bloc has been instantiated and as was pointed out, the underlying implementation isn't affected by this change (we're still seeding the _state of the bloc in both cases). Would be good to move this discussion to #1304 for better visibility 👍

@felangel
Copy link
Owner Author

@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 👍

@felangel felangel requested a review from jorgecoca June 16, 2020 16:48
chimon2000
chimon2000 previously approved these changes Jun 16, 2020
View reviewed changes
jorgecoca
jorgecoca previously approved these changes Jun 16, 2020
View reviewed changes
@felangel felangel changed the title feat!: bloc to extend cubit instead of stream feat!: v5.0.0 Jun 21, 2020
@felangel felangel dismissed stale reviews from jorgecoca and chimon2000 via d1b1c4a June 21, 2020 03:21
@felangel felangel added this to the bloc-v5.0.0 milestone Jun 21, 2020
@felangel felangel added breaking change Enhancement candidate would introduce a breaking change release candidate Candidate for release pending successful build labels Jun 23, 2020
@felangel felangel force-pushed the feat/bloc-extends-cubit branch 2 times, most recently from 7d3abe9 to b617e4f Compare June 24, 2020 16:10
@felangel felangel added pkg:angular_bloc This issue is related to the angular_bloc package pkg:bloc_test This issue is related to the bloc_test package pkg:flutter_bloc This issue is related to the flutter_bloc package labels Jun 28, 2020
@felangel felangel merged commit d70f002 into master Jul 2, 2020
bloc automation moved this from In progress to Done Jul 2, 2020
@felangel felangel deleted the feat/bloc-extends-cubit branch July 2, 2020 17:25
@zenkog
Copy link
zenkog commented Jul 4, 2020 •
edited
Loading

I think this is a good change. Great work @felangel !!! Thank you !! Lots of Thumbs Up!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@narcodico narcodico narcodico left review comments

@zepfietje zepfietje zepfietje requested changes

@jorgecoca jorgecoca jorgecoca left review comments

@chimon2000 chimon2000 chimon2000 left review comments

Assignees

@felangel felangel

Labels
breaking change Enhancement candidate would introduce a breaking change enhancement candidate Candidate for enhancement but additional research is needed feedback wanted Looking for feedback from the community pkg:angular_bloc This issue is related to the angular_bloc package pkg:bloc_test This issue is related to the bloc_test package pkg:bloc This issue is related to the bloc package pkg:flutter_bloc This issue is related to the flutter_bloc package release candidate Candidate for release pending successful build
Projects
bloc
  
Done
Milestone
bloc-v5.0.0
6 participants
@felangel @zepfietje @narcodico @zenkog @jorgecoca @chimon2000