marcglasberg / async_redux

Flutter Package: A Redux version tailored for Flutter, which is easy to learn, to use, to test, and has no boilerplate. Allows for both sync and async reducers.
Other
234 stars 40 forks source link

StoreConnector not being rebuilt if ViewModel includes list of enums #63

Closed pitazzo closed 4 years ago

pitazzo commented 4 years ago

Hey there, I may have found a bug, or maybe I've some errors in my implementation. I've created following connector:

class MiniFilterDisplayConnector extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return StoreConnector<AppState, ViewModel>(
      model: ViewModel(),
      distinct: true,
      builder: (context, model) => MiniFilterDisplay(
        activeCriteria: model.appliedCriteria,
        onCriteriaRemoved: model.onCriteriaRemoved,
      ),
    );
  }
}

class ViewModel extends BaseModel<AppState> {
  ViewModel();

  List<CriteriaKind> appliedCriteria;
  Function(CriteriaKind) onCriteriaRemoved;

  ViewModel.build({
    @required this.appliedCriteria,
    @required this.onCriteriaRemoved,
  }) : super(equals: [appliedCriteria]);

  @override
  BaseModel fromStore() => ViewModel.build(
        appliedCriteria: state.appliedCriteria,
        onCriteriaRemoved: (criteria) {
          dispatch(RemoveAndCopyCriteriaAction(criteriaKindToRemove: criteria));
          dispatch(ApplyFilterAction());
        },
      );
}

It should be rebuilt each time the contents of the list appliedCriteria change. However, the child widget isn't being rebuilt as that list changes. I'm pretty sure about that because:

I think the problem is related somehow to the fact that the list is a list of an enum type, and those lists, in Dart, have to be compared using listEquals() function.

I've tried to implement my own == ViewModel's operator instead of using the super(equals: [appliedCriteria]) thing, like this:

  @override
  int get hashCode => this.appliedCriteria.hashCode;

  @override
  bool operator ==(Object other) {
    return (identical(this, other) ||
        other is ViewModel &&
            listEquals(this.appliedCriteria, other.appliedCriteria));
  }

but it didn't work neither. I've used that ViewModel approach many times in my app with other types in the super(equals: [...]) function and it always worked fine, so I think it should be defineltly related to the fact of being an enum list. Any comment or idea will be helpful!

marcglasberg commented 4 years ago

No, it should not be rebuilt each time the contents of the appliedCriteria change, because appliedCriteria should not change at all. It must not change. All state in a Redux store must be immutable. That's one of the core principles of Redux (not only of AsyncRedux).

Instead, it will rebuilt each time a new appliedCriteria is created.

Please show me the code of your RemoveAndCopyCriteriaAction. I'd guess that's probably mutating appliedCriteria by doing something like appliedCriteria.remove(criteria);. What it should be doing is creating another list, for example: newAppliedCriteria = List.of(appliedCriteria).remove(criteria); and then: return state.copy(appliedCriteria: newAppliedCriteria);

pitazzo commented 4 years ago

With "change" I meant, each time the value of appliedCriteria is different in the current state and in the previous one. Here's my implementation of RemoveAndCopyCriteriaAction, I think it follows the Redux standard, but I'm quite new to Redux so I'm not 100% sure.


class RemoveAndCopyCriteriaAction extends ReduxAction<AppState> {
  final CriteriaKind criteriaKindToRemove;

  RemoveAndCopyCriteriaAction({@required this.criteriaKindToRemove});

  @override
  AppState reduce() {
    return state.copy(
      filterCriteria: state.filterCriteria.copy(
        activeCriteria: state.appliedCriteria..remove(criteriaKindToRemove)
      ),
    );
  }
}
marcglasberg commented 4 years ago

As I said, you are mutating appliedCriteria here: activeCriteria: state.appliedCriteria..remove(criteriaKindToRemove)

Try this instead: activeCriteria: List.of(state.appliedCriteria)..remove(criteriaKindToRemove)

Also, another option for you is using an immutable List to start with. Then you would not be able to mutate the list and create this sort of problems. This package has immutable collections: https://pub.dev/packages/kt_dart

marcglasberg commented 4 years ago

@Pitazzo I closed this issue, but feel free to ask if it still doesn't work or you have any more questions.

pitazzo commented 4 years ago

Thanks for your answer! It worked great! I'm opening again just for clarification: I thought that using ..remove returned the value of the list after the removal, and just copying its value with an implementation of the .copy(...) in the parent objects would do the trick. What is the difference between that and using List.of(...). Thanks once again for your time!

marcglasberg commented 4 years ago

..remove(...) doesn't create a new list. I just mutates the current list, by removing one item. However, that same list is referenced by both the old state and the new state. The list is effectively the same object in both states.

When you do List.of(...) it creates a new copy of the list, which is a different object than the first one. Then when you do List.of(...)..remove(...) you are mutating the new list, not the old one. Now, your new state ends up with a different list from the previous state.

I suggest that you further study about immutability in general. Understanding immutability is a precondition to understanding Redux.