RedBrogdon / rebloc

A state management library for Flutter that combines aspects of Redux and BLoC.
BSD 3-Clause "New" or "Revised" License
215 stars 21 forks source link

don't update the store were cancelled action returned by the reducer #15

Open r3flow opened 5 years ago

r3flow commented 5 years ago

a quick fix:

--- engine.dart
+++ engine.dart
@@ -104,8 +104,10 @@

     reducerStream.listen((a) {
       assert(a.state != null);
-      states.add(a.state);
-      _afterwareController.add(WareContext<S>(dispatcher, a.state, a.action));
+      if (a.action is! _CancelledAction) {
+        states.add(a.state);
+        _afterwareController.add(WareContext<S>(dispatcher, a.state, a.action));
+      }
     });

     // Without something listening, the afterware won't be executed.
RedBrogdon commented 5 years ago

I'm not sure that this would be a good idea, though I could be convinced otherwise.

_CancelledAction is designed to be an Action type that an app developer's middle-/afterware and reducers can't check for when processing actions. The type isn't exported by the package, so you can't just do something like if (action is _CancelledAction) { ... } without getting an error.

I have assumed (and I admit it's an assumption) that app devs write reducers that look like this:

  @override
  AppState reducer(state, action) {
    if (action is SomeSpecificAction) {
      // Return state, modified for the above action.
    } else if (action is AnotherSpecificAction) {
      // Return state, modified for the above action.
    }

    // Return unmodified state if the action isn't recognized by type.
    return state;
  }

Because devs can't specifically test for _CancelledAction and it doesn't really make sense to update the state for any old action that comes by, instances of _CancelledAction should pass through the reducer stream without causing the app state to change.

For that reason, it still seems cleaner to me to leave things as they are rather than declare _CancelledAction the one type of Action that doesn't go through the reduce/afterware portion of the dispatch stream.

r3flow commented 5 years ago

Okay, I understand that you wouldn't like to distinguish the _CancelledAction from other Actions, but if we look closely it is might be already different because this type isn't exported by the package.

Perfectly fine that this type isn't exported by the package, and my reducers looks exactly like as you assumed. I have no intention to catch the _CancelledAction in any reducer (and not in any afterware), _CancelledAction is "invisible" to them, as you mentioned (and that's fine).

So, _CancelledActions are "invisible" but "heavy". Because (unless I missed some detail) it will call the every reducers (there may be a lot of them), then call the every middlewares, then the (unchanged) state (throught the BehaviorSubject stream) will call the every ViewModelSubscriber's converter and all of them compare the unchanged (but might be recreated) view models.

Of course, it's not a critical problem beacause the _CancelledAction is very rare guest (it's my assumption).

The current behavior is fine for me, I would have liked reducing the load of the main thread.

RedBrogdon commented 5 years ago

You've got it right, and the criticism is fair. If you'd really like to avoid the extra work done as _CancelledAction makes its way through the dispatch stream, though, you can always implement Bloc directly, rather than using SimpleBloc. Blocs that implement the actual Bloc interface are wired directly into the dispatch stream and can simply refuse to emit an action at all when they see something they want to cancel.

It's also possible that I could rework the SimpleBloc class so that _CancelledAction isn't necessary, and the middleware-type methods in SimpleBloc return a boolean or something to indicate that an Action should continue down the stream. I'll have a think on it.

In the meantime, thanks for posting an issue. I'm always happy to see someone caring enough to start the discussion. 😄