felangel / bloc

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

Allow for synchronous dispatch to use blocs in blocs #238

Closed droidpl closed 5 years ago

droidpl commented 5 years ago

Is your feature request related to a problem? Please describe. In examples like the login, its suggested that we can have blocs inside blocs, like the authentication bloc inside the login bloc, and then we can call dispatch from one to another. Currently the dispatch is asynchronous thus you are calling another bloc and expect it to do whatever job.

The problem is, in the UI you have to listen multiple blocs at the same time to handle your state properly as the states might change in each of them. A valid use case that shows this would be te create account bloc, going as follow:

If I do it with the current dispatch, the signin attempt will happen after the creation, but I need to listen to a completely different state to make sure the authentication is fully completed, complicating quite a lot the code, while I could just listen for a CreateAccountCompleted

Describe the solution you'd like If there would be a synchronous dispatch for a certain action from one bloc to another, I could wait for an action to happen obtaining a proper result and I can act in my container bloc to simplify the UI layer.

Describe alternatives you've considered I considered using some sort of a widget that provides states combined, or just chaining them, but both seem to complicate more the flows than the proposed one.

Of course other option is not using the bloc, but the repository, but that would mean I am not reusing already the logic of the bloc states.

felangel commented 5 years ago

Hi @droidpl πŸ‘‹ Thanks for opening an issue!

Regarding your feature request, can you please share a code snippet for how the dispatch usage would change? I just want to make sure I fully understand your feature request.

In the meantime, I have spent considerable time trying to make dispatches "awaitable" so that you can be sure the dispatch has been processed before executing some other code. The problem with this is a single event can trigger multiple state changes. This is a feature of the bloc package which is really powerful in my opinion because you can yield a loading state, do some async processing, and yield a success state in response to a single event.

This behavior/functionality makes it impossible to await a dispatch because there's no way to know when the dispatch is "done". You cannot assume that because the state has changed once that the dispatch has been fully processed.

As a result, I would recommend using BlocListener and dispatch events to other blocs in response to state changes in a given bloc.

Let me know if that makes sense and if I understood your question correctly πŸ‘

droidpl commented 5 years ago

Hi @felangel,

I get your point regarding the BlocListener (which btw, discovered today πŸ’ƒ I was using the 0.9.0). I was actually talking about awaiting a dispatch. Although the BlocListener would work, its bringing some complexity to the UI part while it can be way easier on the blocs.

The signature as I think it would be it's a method which returns a future of a state type and that is awaitable. Kind of:

Future<State> dispatchAwaitable(Event event) async {
    //Some magic here
}

The state that is provided would be the latest state after the mapEventToState execution. Of course the yields still should report to the proper Subject for the rest of subscribed listeners.

About implementation and ideas, I didn't mature this enough, but looking at the current code doesn't seem trivial. Maybe a special subject that replicates to the other one and as a finishing event reports something special flag to indicate a return, maybe I am just saying weird things. I have some experience with RxJava, but not that much with rxDart. Tomorrow I can look a bit more in deep into it and see if I can come with some idea.

PD: I didn't came up with a better name than dispatchAwaitable, I am sure there is something better.

droidpl commented 5 years ago

I worked on a small prototype for the functionality. Let me share it with you so you can also give some thoughts on it. The idea is to create an awaitable dispatch method, that you can wait as a future providing the last state reached.

The streams:

And here the code:

abstract class AwaitableBloc<Event, State> {
  Future<State> awaitableDispatch(Event event) async {
    BehaviorSubject<Closeable<State>> subject = BehaviorSubject();
    subject.addStream(eventToStateWrapper(event));
    Completer completer = Completer<State>();
    State lastState;
    subject.listen((elem) {
      print("State reached ${elem.item}");
      if(elem.isCloseEvent()){
        completer.complete(lastState);
      }
      lastState = elem.item;
    });
    return completer.future;
  }

  Stream<Closeable<State>> eventToStateWrapper(Event event) async* {
    yield* mapEventToState(event).map((item) => Closeable(item));
    yield Closeable<State>(null);
  }

  Stream<State> mapEventToState(Event event);
}

class Closeable<T> {
  T item;

  Closeable(this.item);

  isCloseEvent() => item == null;
}

And running a test:

//Running the sample test
class Sample1 extends AwaitableBloc<int, int> {
  Sample2 sample;

  Sample1(this.sample);

  @override
  Stream<int> mapEventToState(int event) async* {
    yield 1;
    yield await Future.delayed(Duration(seconds: 1), () => 2);
    yield await sample.awaitableDispatch(4);
    yield event;
  }

}

class Sample2 extends AwaitableBloc<int, int> {
  @override
  Stream<int> mapEventToState(int event) async* {
    yield 3;
    yield await Future.delayed(Duration(seconds: 1), () => event);
  }
}

_runTest() async {
    var sample1 = Sample1(Sample2());
    print(await sample1.awaitableDispatch(0)); //Should print 0
}

This prints:

flutter: State reached 1
flutter: State reached 2
flutter: State reached 3 -- awaits here the second dispatch
flutter: State reached 4
flutter: State reached null
flutter: State reached 4 -- returns here the second dispatch
flutter: State reached 0
flutter: State reached null
flutter: 0 -- final dispatch with 0

Let me know your thoughts.

felangel commented 5 years ago

@droidpl thanks for the detailed implementation. I don't think this approach would work for all cases.

Take the following example:

_runTest() async {
    var sample1 = Sample1(Sample2());
    sample1.awaitableDispatch(0);
    print(await sample1.awaitableDispatch(0)); //Should print 0
}

In this case, you would lose the first completer because you would overwrite it on the second awaitableDispatch.

Furthermore, if developers wanted to do something like:

await Future.wait([
  sample1.awaitableDispatch(0),
  sample1.awaitableDispatch(0),
]);

It would also not work as expected for the same reason.

Thoughts?

droidpl commented 5 years ago

Why it wouldn't work? If a new BehaviourSubject is generated for each awaitable dispatch, those will be handled independently. Maybe I am missing something there, can you post the results you get and the expected ones?

Thanks for having a look to this

felangel commented 5 years ago

Hey @droidpl that was my mistake!

After further discussion and investigation, we have decided that awaitable dispatches would complicate the library and also violate the pattern of events in and states out. I don't think it makes sense to try to incorporate this change into the library and would love to try to help with your specific issue (if you'd like) as opposed to trying to integrate this functionality.

I really appreciate the effort you've put into your proposal and for helping the flutter open source community πŸ‘

Hope that makes sense and let me know what you think! Closing this for now.

droidpl commented 5 years ago

Hi @felangel,

Oh sad to hear it. So the case I am trying to handle is similar to the tutorial you have on the documentation, but combining signup. Basically the flow is the following:

Of course I can have a BlocListener for the authentication bloc to know if I am authenticated, and the happy path would work ok. My problem is the unhappy paths, as it might fail the login, registration or authorization blocs, and handling all those different errors asynchronously leaves a way harder to maintain/read/implement code. If I would be able to call one bloc to the other, I could use the single listener I have for the signup bloc to handle the states, without cluttering the API with 3 BlocListeners for each of the possible errors and fallback handling.

Other option is just use the repository to handle that behavior, but then I would repeat part of the code I have in the login/authentication bloc, which would be also a bad practice.

Hope it's a bit more clear what I mean. I can also give you some code to see what I mean :). And thanks for taking into consideration the request.

droidpl commented 5 years ago

Actually, I had another idea that could also work and probably fit the library proposals, is it possible to subscribe from one bloc to others and receive the events there + dispatch states on the current bloc?

I mean:

That would solve the best of both worlds, I don't have to repeat the code, keep the business logic on the blocs and still I am able to dispatch from my current one.

Didn't thought on the implementation of it, but it must be way simpler than the one proposed before and matches the pubsub criteria you mentioned. Give it a thought and let me know πŸ˜„

droidpl commented 5 years ago

@felangel Did you have a chance to have a look to this?

felangel commented 5 years ago

@droidpl sorry for the delay; I just saw this. I think what you're describing is currently possible and documented in bloc to bloc communication. Let me know if that's what you were thinking πŸ‘

droidpl commented 5 years ago

oh I didn't see that documentation. Only thing is I can't yield states there, but I guess just by dispatching new events I can trigger the state I want. Not the most comfortable way but still would work.

Thanks for the support and the nice library.

felangel commented 5 years ago

No problem! I’ll keep it in mind and hopefully have a more elegant solution in the near future. πŸ‘