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

feat(bloc)!: expose Emitter in Cubit #2896

Closed
wants to merge 6 commits into from

Conversation

felangel
Copy link
Owner

@felangel felangel commented Oct 22, 2021

Status

IN DEVELOPMENT

Breaking Changes

YES

Description

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

@felangel felangel added enhancement New feature or request pkg:bloc This issue is related to the bloc package breaking change Enhancement candidate would introduce a breaking change refactor Refactor an existing implementation labels Oct 22, 2021
@felangel felangel self-assigned this Oct 22, 2021
@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #2896 (44a86b4) into feat/v8.0.0 (71c332f) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff               @@
##           feat/v8.0.0     #2896    +/-   ##
==============================================
  Coverage       100.00%   100.00%            
==============================================
  Files               25         6    -19     
  Lines              639       261   -378     
==============================================
- Hits               639       261   -378     
Impacted Files Coverage Δ
packages/bloc/lib/src/bloc.dart 100.00% <100.00%> (ø)
packages/flutter_bloc/lib/src/bloc_selector.dart
packages/bloc_concurrency/lib/src/concurrent.dart
packages/bloc_test/lib/src/mock_bloc.dart
...ages/flutter_bloc/lib/src/repository_provider.dart
packages/hydrated_bloc/lib/src/hydrated_bloc.dart
packages/flutter_bloc/lib/src/bloc_listener.dart
...ackages/hydrated_bloc/lib/src/hydrated_cipher.dart
packages/flutter_bloc/lib/src/bloc_provider.dart
... 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 71c332f...44a86b4. Read the comment docs.

@felangel felangel marked this pull request as draft October 22, 2021 04:07
@felangel
Copy link
Owner Author

@jeroen-meijer @definitelyokay after thinking about it I'd prefer not to support the Emitter API for Cubit for several reasons:

  • emit.forEach and emit.onEach while functional in Cubit will not be cancelable which imo reduces the usefulness significantly
  • The new API opens the door to developers being able to cache a reference of an Emitter since emit would be a getter
  • isDone only makes sense in the context of Bloc but is part of the base Emitter interface so we'd either need to have a specific BlocEmitter and CubitEmitter or Cubit users would have access to a useless property.

Would love to hear what you all think, thanks!

@jolexxa
Copy link
Collaborator

jolexxa commented Oct 22, 2021

  • The new API opens the door to developers being able to cache a reference of an Emitter since emit would be a getter
  • isDone only makes sense in the context of Bloc but is part of the base Emitter interface so we'd either need to have a specific BlocEmitter and CubitEmitter or Cubit users would have access to a useless property.

You're absolutely right. It was a nice idea, but I guess it's bad in practice. Didn't foresee that, but live and learn 🤷‍♀️

@felangel felangel closed this Oct 23, 2021
@felangel felangel deleted the feat/bloc-cubit-emitter branch October 23, 2021 06:45
@felangel felangel added the wontfix This will not be worked on label Oct 23, 2021
@marctan
Copy link

marctan commented Feb 7, 2022

Hello, sorry for bumping this thread., but if I want to listen to a stream in Cubit class is below correct?

class ChatCubit extends Cubit<ChatState> {
  ChatCubit({required this.repository}) : super(const ChatState());
  final ChatRepository repository;
 StreamSubscription? subscription;

  @override
  Future<void> close() {
    subscription?.cancel();
    return super.close();
  }

void getMessages(String recipient) async {
    emit(state.copyWith(status: ChatStatus.messagesLoading));
    await subscription?.cancel();

    subscription = repository.getMessageStream(recipient).listen((event) {
      final List<ChatMessage> messageList = event.docs
          .map(
            (e) => ChatMessage(
              text: e['message'],
              user: ChatUser(
                uid: e['sender'],
                name: e['sender'],
              ),
              createdAt: DateTime.fromMillisecondsSinceEpoch(
                e['time'],
              ),
            ),
          )
          .toList();
      emit(state.copWith(status: ChatStatus.messagesLoaded, messages: messageList));
    });
  }
}

Thanks!
@felangel

@Gene-Dana
Copy link
Collaborator

Feel free to put the subscription in the constructor body too!

@zamaniafshar
Copy link

  • emit.forEach and emit.onEach while functional in Cubit will not be cancelable which imo reduces the usefulness significantly
  • The new API opens the door to developers being able to cache a reference of an Emitter since emit would be a getter
  • isDone only makes sense in the context of Bloc but is part of the base Emitter interface so we'd either need to have a specific BlocEmitter and CubitEmitter or Cubit users would have access to a useless property.

I think the main reason this hasn’t been done yet is due to implementation challenges, not because adding these features to Cubit is a bad idea.
I really hope this feature is added to Bloc soon, as it’s a big pain to have to use Bloc in many situations just because of a few streams—which feels unreasonable. In the meantime, I've created a solution for myself.
#3555 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Enhancement candidate would introduce a breaking change enhancement New feature or request pkg:bloc This issue is related to the bloc package refactor Refactor an existing implementation wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants