Bug in the view-model creation: Method _ignoreChange may be called with a different state than _mapConverter.

[marcglasberg]

Some user from AsyncRedux pointed me to this potential problem, and since it is totally based upon flutter_redux, I just checked, and the problem exists in both.

From redux.dart (https://github.com/fluttercommunity/redux.dart/blob/master/lib/src/store.dart):

_state = state;
_changeController.add(state);

Then later, in flutter_redux.dart (https://github.com/brianegan/flutter_redux/blob/master/lib/flutter_redux.dart):

stream = widget.store.onChange
        .where(_ignoreChange)
        .map(_mapConverter)

Note _mapConverter uses the stream state:

  bool _ignoreChange(S state) {
    if (widget.ignoreChange != null) {
      return !widget.ignoreChange(state);
    }
    return true;
  }

Problem is, _mapConverter does NOT actually use the stream state. Instead, it uses the STORE state:

  ViewModel _mapConverter(S state) {
    return widget.converter(widget.store);
  }

But the store state is not guaranteed to be the same as the stream state. The stream is async, so at any given moment the store may hold a slightly newer state.

This means _ignoreChange may decide you should not ignore some state (because it is seeing a potentially older state),
but the store than fails to create the view-model because it is using the current state. In this case, the _ignoreChange
never had the chance to veto the view-model creation. This is a potential app crash, or at least the view-model will be wrong.

[marcglasberg]

This is the code to reproduce the problem. Each time you click the plus button it increments 5 times:

converter: (store) {
      return () {
              store.dispatch(Actions.Increment);
              store.dispatch(Actions.Increment);
              store.dispatch(Actions.Increment);
              store.dispatch(Actions.Increment);
              store.dispatch(Actions.Increment);
             };},

We try to ignore screen updates when the count is odd:

ignoreChange: (count) {
      var isOdd = (count % 2 == 1);
      print('ignoreChange: $isOdd (count = ${count})');
      return isOdd;
},

But it doesn't work. It updates everytime and display the odd numbers: 0, 5, 10, 15, 20, 25, 30, 35, 40..

import 'package:flutter/material.dart';
import 'package:flutter_redux/flutter_redux.dart';
import 'package:redux/redux.dart';

enum Actions { Increment }

int counterReducer(int state, dynamic action) {
  if (action == Actions.Increment) return state + 1;
  return state;
}

void main() {
  final store = Store<int>(counterReducer, initialState: 0);
  runApp(FlutterReduxApp(store: store));
}

class FlutterReduxApp extends StatelessWidget {
  final Store<int> store;
  final String title;

  FlutterReduxApp({Key key, this.store, this.title}) : super(key: key);

  @override
  Widget build(BuildContext context) {
    return StoreProvider<int>(
        store: store,
        child: MaterialApp(
            title: 'Example',
            home: Scaffold(
                appBar: AppBar(title: const Text('Example')),
                body: Center(
                  child: Column(
                    mainAxisAlignment: MainAxisAlignment.center,
                    children: [
                      Text(
                        'Each time you click it increments 5 times. '
                        'It should ignore odd results, but it does not:',
                      ),
                      StoreConnector<int, String>(
                        converter: (store) => store.state.toString(),
                        ignoreChange: (count) {
                          var isOdd = (count % 2 == 1);
                          print('ignoreChange: $isOdd (count = ${count})');
                          return isOdd;
                        },
                        builder: (context, count) {
                          return Text(count, style: Theme.of(context).textTheme.display1);
                        },
                      )
                    ],
                  ),
                ),
                floatingActionButton: StoreConnector<int, VoidCallback>(
                  converter: (store) {
                  // Increments 5 times.
                  return () {
                    store.dispatch(Actions.Increment);
                    store.dispatch(Actions.Increment);
                    store.dispatch(Actions.Increment);
                    store.dispatch(Actions.Increment);
                    store.dispatch(Actions.Increment);
                  };
                }, builder: (context, callback) {
                  return FloatingActionButton(
                    onPressed: callback,
                    child: Icon(Icons.add),
                  );
                }))));
  }
}

[marcglasberg]

To fix the problem, in the Store, instead of this:

bool _ignoreChange(S state) {
    if (widget.ignoreChange != null) {
      return !widget.ignoreChange(state);
    }

    return true;
  }

It could be doing this:

bool _ignoreChange(S state) {
    if (widget.ignoreChange != null) {
      return !widget.ignoreChange(widget.store.state); // Use the store state.      
    }

    return true;
  }

See: PR #198

However, this fix is incomplete. It will now display: 0, 10, 20, 30, 40... while ideally it could display: 0, 4, 10, 14, 20, 24, 30, 24, 40...

I have solved this properly for AsyncRedux, and can propose a PR with the same solution here. However it is not pretty. You have to save the last valid state from both the stream and the store, and then calculate the view-model from this valid state when the store state breaks.

[brianegan]

Fixed by PR