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.

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

refactor(bloc): Extract base bloc interface #2453

Closed
wants to merge 3 commits into from

Conversation

gocal
Copy link

@gocal gocal commented May 9, 2021

Currently BlocBase is an abstract class so it's not possible to easily extend bloc functionality e.g. via custom mixins, Instead we always have to extend all the available implementations (bloc, cubit). Having the actual interface/contract instead of the abstract class guarantees bloc/cubit is independent from the BlocBase implementation.

I've create a small extensions to the bloc library that allows to use side effects that don't requires widget tree to be rebuilt.

https://github.com/allbrightio/effect_bloc/blob/main/examples/flutter_navigation/lib/view/login/login_bloc.dart

Status

READY

Breaking Changes

NO

Description

Converted BlocBase to an interface, and added default implementation of it (_BlocBaseImpl).
Since _BlocBaseImpl is not exposed outside this is not a breaking change.

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

@gocal gocal marked this pull request as ready for review May 10, 2021 06:00
@gocal gocal requested a review from felangel as a code owner May 10, 2021 06:00
@codecov
Copy link

codecov bot commented May 10, 2021

Codecov Report

Merging #2453 (b4b10ec) into master (63cebe5) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            master     #2453    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           17         3    -14     
  Lines          444        99   -345     
==========================================
- Hits           444        99   -345     
Impacted Files Coverage Δ
packages/bloc/lib/src/bloc.dart 100.00% <100.00%> (ø)
...ckages/hydrated_bloc/lib/src/hydrated_storage.dart
...ages/flutter_bloc/lib/src/repository_provider.dart
packages/flutter_bloc/lib/src/bloc_provider.dart
...ages/flutter_bloc/lib/src/multi_bloc_listener.dart
packages/flutter_bloc/lib/src/bloc_builder.dart
packages/flutter_bloc/lib/src/bloc_listener.dart
packages/flutter_bloc/lib/src/bloc_consumer.dart
...lutter_bloc/lib/src/multi_repository_provider.dart
packages/bloc_test/lib/src/mock_bloc.dart
... and 5 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 38e720e...b4b10ec. Read the comment docs.

@felangel felangel added pkg:bloc This issue is related to the bloc package refactor Refactor an existing implementation labels May 10, 2021
@felangel
Copy link
Owner

felangel commented May 29, 2021

@gocal thanks for the contribution! I'll try to get this merged shortly but unfortunately, I believe it is a breaking change because developers could be extending BlocBase in their applications and overriding one or more methods. These changes would be breaking for those scenarios because developers would have to fully implement the BlocBase interface from scratch.

@felangel
Copy link
Owner

Going to close this PR since it's quite out-dated and has lots of merge conflicts -- this will be addressed as part of #2896 and included in the upcoming v8.0.0 release. Thanks again!

@felangel felangel closed this Oct 22, 2021
@felangel
Copy link
Owner

felangel commented Oct 23, 2021

@gocal sorry for the delay but I'm revisiting this and I was hoping you could clarify the issue you were facing with the current implementation.

Currently BlocBase is an abstract class so it's not possible to easily extend bloc functionality e.g. via custom mixins, Instead we always have to extend all the available implementations (bloc, cubit). Having the actual interface/contract instead of the abstract class guarantees bloc/cubit is independent from the BlocBase implementation.

There is no interface keyword in Dart, instead abstract classes can be used as interfaces to be extended or implemented by other classes. Currently, Bloc and Cubit extend BlocBase which is an abstract class that defines the public API. You can create custom mixins on BlocBase which can be used with both Bloc and Cubit (since they both extend BlocBase).

You should be able to define an EffectBloc mixin like:

mixin EffectBloc<State, Effect> on BlocBase<State> {
  StreamController<Effect>? __effectController;
 
  StreamController<Effect> get _effectController {
    return __effectController ??= StreamController<Effect>.broadcast();
  }

  Stream<Effect> get effectStream => _effectController.stream;

  void emitEffect(Effect effect) {
    if (_effectController.isClosed) return;
    onEffect(effect);
    _effectController.add(effect);
  }

  @mustCallSuper
  void onEffect(Effect effect) {}

  @mustCallSuper
  @override
  Future<void> close() async {
    await _effectController.close();
    return super.close();
  }
}

Then apply the mixin like:

class MyEffectBloc extends Bloc<MyEffectEvent, MyEffectState>
    with EffectBloc<MyEffectState, EffectType> {
      // ...
}

Let me know if that helps or if I misunderstood, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:bloc This issue is related to the bloc package refactor Refactor an existing implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants