rrousselGit / state_notifier

ValueNotifier, but outside Flutter and with some extra perks
MIT License
311 stars 28 forks source link

Unclear on the correct approach for updated multiple values of state object (with freezed data class) #36

Closed scopendo closed 4 years ago

scopendo commented 4 years ago

When using StateNotifier with a freezed state class, there is a need to add a setter for each state attribute that should be modifiable since the notifier's state is protected. This is cumbersome when there are many state attributes, but also it feels wrong adding what is essentially a proxy method for mutating the state on a per attribute basis.

@freezed
abstract class MyData implements _$MyData {
  const factory MyData(String foo, String bar) = _MyData;
}

class MyDataNotifier extends StateNotifier<MyData> {
  MyDataNotifier() : super(null);

  set foo(String foo) => state = state.copyWith(foo: foo);

  set bar(String bar) => state = state.copyWith(bar: bar);
}

This also means that modifying multiple state properties requires multiple passes of the copyWith, which seems inefficient – though perhaps is immaterial.

final myDataNotifier = MyDataNotifier(foo: 'abc', bar: 'xyz');

myDataNotifier.foo = '123'; // creates a new state object
myDataNotifier.bar = '456'; // creates a new state object again

One way around this, although seemingly against the philosophy of StateNotifier, is to expose a method that accepts a function that returns an updated state given the current state:

class MyDataNotifier extends StateNotifier<MyData> {
  MyDataNotifier() : super(null);

  // Allow consumer to update state without needing to have the state first
  void update(MyData Function (MyData) updater) => state = updater(state);
}

final myDataNotifier = MyDataNotifier(foo: 'abc', bar: 'xyz');

myDataNotifier.update((state) => state.copyWith(foo = '123', bar = '456'));

I'm wondering whether there is any guidance on the best way to be using StateNotifier, particularly with freezed, when multiple attributes need to be updated in one section of code.

rrousselGit commented 4 years ago

there is a need to add a setter for each state attribute that should be modifiable since the notifier's state is protected.

Don't do that. You shouldn't have setters in your class. That's for ChangeNotifiers.

Instead expose meaningful methods on the object, which regroups all the mutations together in a single clone.

@freezed
abstract class MyData implements _$MyData {
  const factory MyData(String foo, String bar) = _MyData;
}

class MyDataNotifier extends StateNotifier<MyData> {
  MyDataNotifier() : super(null);

  void doSomething({@required String foo, @required String bar}) {
    state = state.copyWith(
      foo: foo,
      bar: bar,
    );
  }
}

void main() {
  final myDataNotifier = MyDataNotifier(foo: 'abc', bar: 'xyz');

  myDataNotifier.doSomething(foo: '123', bar: '456');
}
scopendo commented 4 years ago

I totally get that when doSomething is actually a discrete piece of domain/business logic. However, when the state is just a data holder for an entity that's being edited, such as with a form UI, then it makes less sense and you end up with a single update method – which I don't mind, but it's not the spirit of StateNotifier.

/// A [StateNotifier] that allows the consumer to update multiple attributes
/// of the state object without already having the state object.
///
/// Adds a default constructor such that the sub-class does not have to
/// define a constructor if the initial state value is to be null.
///
abstract class MyStateNotifier<T> extends StateNotifier<T> {
  MyStateNotifier([T initialState]) : super(initialState);

  /// Updates the notifier's state to the value returned by the updater
  /// function.
  ///
  /// Passes in the current state value.
  ///
  update(T Function(T) updater) {
    this.state = updater(this.state);
  }
}

class MyDataNotifier extends MyStateNotifier<MyData> {
}

final myDataNotifier = MyDataNotifier();

myDataNotifier.update((state) => state.copyWith(foo: '123', bar: '456');

As an aside @rrousselGit, thank you once again for not only your packages but also your active support of people using them.

rrousselGit commented 4 years ago

I'm not sure what is the relationship with forms

scopendo commented 4 years ago

Suppose that the state class was:

@freezed
abstract class Person implements _$Person {
  const factory Person(
    String name,
    GenderEnum gender,
    DateTime dateOfBirth,
    String nickname,
    String primaryEmail,
  ) = _Person;

If we had a simple UI that wanted to allow a Person entity to be edited, then we might just put a bunch of corresponding form fields on the UI and then when save is invoked, we'd update the state for the person entity according to the input values.

rrousselGit commented 4 years ago

But then with a form you are not modifying multiple properties at the same time as you have a field per property So I'm not sure what the problem is

scopendo commented 4 years ago

I'd need to add a setter per property, or I could update all the properties in one go (e.g. from text controller values), such as in a Save button's onPress callback.

rrousselGit commented 4 years ago

You can make "state" public if you want to.