felangel / bloc

A predictable state management library that helps implement the BLoC design pattern
https://bloclibrary.dev
MIT License
11.72k stars 3.38k forks source link

BlocListener no longer observes first state #404

Closed dustin-graham closed 5 years ago

dustin-graham commented 5 years ago

Describe the bug

370 packages/flutter_bloc/lib/src/bloc_listener.dart line adds a skip(1) operator to the bloc listener. The commit indicates this is intentional so as to only respond to state updates. This ended up breaking one of my widget tests where I mock the provided bloc to provide the state update my app needs to react to as the first event in its stream. To fix my test I had to add an arbitrary "1st" event which works, but seems a bit contrived. It seems to me that the first state should count as a state change as the stream changes from no events to one event.

Given #370, this appears to be new expected behavior. However, based on #213, observing the first event was previously intended. Is there a reason why the first state needs to be skipped by the bloc observer?

Expected behavior The BlocListener observes the first state

basketball-ico commented 5 years ago

@dustin-graham Hi Please check this https://github.com/felangel/bloc/issues/368

dustin-graham commented 5 years ago

I just found #368. I get the case where you are trying to only call this function when state changes and the situation we avoid by the skip is confusing the consequence of the listener subscription with actual state updates.

Could we perhaps use BehaviorSubject's hasValue function to determine if we should drop the initial event?

dustin-graham commented 5 years ago

I think there should be a way to keep the fix made in #368 and still include the very first state.

felangel commented 5 years ago

Hey @dustin-graham 👋 Thanks for opening an issue!

Regarding your request, by definition BlocListener should be called once for each state change and I don't think the initialState is considered a state change since the bloc defines it's own initialState.

As a result, I think it's unnecessary to have BlocListener be triggered on initialState and it also introduces lots of undesired behavior such as being re-triggered on initialization of widgets even though no state has occurred (as described in #368).

Using the BehaviorSubject's hasValue is also not viable because the BehaviorSubject is seeded with initialState so hasValue will always evaluate to true.

I would love to hear more about why you think this is necessary because instead of

BlocListener(
  bloc: myBloc,
  listener: (context, state) {
    if (state is InitialState) {
      doStuff();
    }
  },
  child: MyChild(),
)

you can just do:

BlocProvider(
  builder: (context) {
    final myBloc = MyBloc();
    doStuff();
    return myBloc;
  },
  child: MyChild(),
)

Let me know if that helps 👍

dustin-graham commented 5 years ago

@felangel , I appreciate the response. I gave this some thought and I think this all makes sense to me now. I appreciate the explanation.

danielborges93 commented 5 years ago

The BlocListener is not listening the first state change.

My states:

enum DisplayOptionsState { waiting, success, failure }

My initial state is:

@override
DisplayOptionsState get initialState => DisplayOptionsState.waiting;

My mapEventToState:

  @override
  Stream<DisplayOptionsState> mapEventToState(SetDisplayTypeEvent event) async* {
    try {
      await api.setDisplayState(event.type);
      yield DisplayOptionsState.success;
    } catch (e) {
      yield DisplayOptionsState.failure;
    }
    yield DisplayOptionsState.waiting;
  }

My listener function:

      listener: (context, state) {
        switch (state) {
          case DisplayOptionsState.success:
            _scaffoldKey.currentState.showSnackBar(SnackBar(
              content: Text('Success'),
            ));
            break;
          case DisplayOptionsState.failure:
            _scaffoldKey.currentState.showSnackBar(SnackBar(
              content: Text('Error!'),
              backgroundColor: Colors.red,
            ));
            break;
          default:
            break;
        }
      },

The idea is after a api call, show a snackBar with success or error message. When I dispatch events two times, the listener responds only to the second. Is this the correct behavior?

felangel commented 5 years ago

Hey @danielborges93 👋

Can you provide a link to a sample app that illustrates the problem you're having? Alternatively, have you checked out the SnackBar Recipe?

BlocListener should be triggered once for each state change 👍

One thing I've noticed is that you're always yielding waiting immediately after either a success or a failure. I'd recommend restructuring your mapEventToState like so:

  @override
  Stream<DisplayOptionsState> mapEventToState(SetDisplayTypeEvent event) async* {
    yield DisplayOptionsState.waiting;
    try {
      await api.setDisplayState(event.type);
      yield DisplayOptionsState.success;
    } catch (e) {
      yield DisplayOptionsState.failure;
    }
  }
danielborges93 commented 5 years ago

Hi @felangel! Thanks for your response!

I have checked the SnackBar Recipe before ;-) I tried your mapEventToState change suggestion, but the behavior was the same.

The app that I am developping is https://gitlab.com/openlp/openlp-mobile-remote (branch api_base). The widget and bloc are in lib/src/widgets/display_options_dialog.dart and lib/src/bloc/display_options_dialog_bloc.dart.

otto-dev commented 2 years ago

So is the solution really to put the business logic for the initial state in the BlocProvider rather than the BlocListener? I was considering that, felt it was a unreliable; googled, and came here.

otto-dev commented 2 years ago

I just ended up here again via google. As I understand now, the issue is actually more general.

The issue is that the listener is not called with the state of the Bloc when the BlocListener is created - no matter if that's the initial state, or a subsequent state. So then you have to refactor out the listener logic and work around it with a StatefulWidget. Bit inconvenient, but also fine.

valas69 commented 6 months ago

The issue arises when you've already fetched all your Bloc data from another service, and you wish to listen for it in another component.

Example

This scenario is just one use case; there could be others. I believe introducing a listenFirst flag could be a good idea to trigger at Bloc listener initialization. This flag would help in managing the flow of data between components more efficiently, especially in scenarios where the data is fetched before the component starts listening to the Bloc as the state does not change.

valas69 commented 6 months ago

The issue arises when you've already fetched all your Bloc data from another service, and you wish to listen for it in another component.

Example

  • Route A: Initializes user data fetch. Once fetched, it redirects to Route B.
  • Route B: Listens to that Bloc and displays a "hello user" snackbar.

This scenario is just one use case; there could be others. I believe introducing a listenFirst flag could be a good idea to trigger at Bloc listener initialization. This flag would help in managing the flow of data between components more efficiently, especially in scenarios where the data is fetched before the component starts listening to the Bloc as the state does not change.

The only workaround I've found is to use Future.delayed(Duration.zero, () { showUserDialog(); }), at the top of the builder of BlocConsumer but I don't find this to be very clean. It would have been better to place it in the listener, in my opinion.

DIRRHUB commented 1 month ago

The issue arises when you've already fetched all your Bloc data from another service, and you wish to listen for it in another component.

Example

  • Route A: Initializes user data fetch. Once fetched, it redirects to Route B.
  • Route B: Listens to that Bloc and displays a "hello user" snackbar.

This scenario is just one use case; there could be others. I believe introducing a listenFirst flag could be a good idea to trigger at Bloc listener initialization. This flag would help in managing the flow of data between components more efficiently, especially in scenarios where the data is fetched before the component starts listening to the Bloc as the state does not change.

The only workaround I've found is to use Future.delayed(Duration.zero, () { showUserDialog(); }), at the top of the builder of BlocConsumer but I don't find this to be very clean. It would have been better to place it in the listener, in my opinion.

We can use WidgetsBinding.instance.addPostFrameCallback in the initState and call an event there.