GIfatahTH / states_rebuilder

a simple yet powerful state management technique for Flutter
494 stars 56 forks source link

[Proposal] allow states to be null because null object pattern redundant with null safety? #184

Closed samuelstroschein closed 3 years ago

samuelstroschein commented 3 years ago

Introduction

My very first opened issue #153 was caused by setting a userState to null to express that no user is logged in which failed silently (also related to #183). @GIfatahTH made me aware that states rebuilder requires a null object pattern as by default states can not be set to null https://github.com/GIfatahTH/states_rebuilder/issues/153#issuecomment-750448963. I would like to start a discussion if forcing the use of null object patterns by disallowing states to be nullable is still adding benefit with the introduction of Dart's null safety.

Problem

States Rebuilder forces developers to avoid null states by using the null object pattern e.g. instead of a authState being null when no user is logged in, the state is of type NoUser (or similar) which is an extension of the User class.

The following example illustrates an authState before the null safety update:

class User {
  String id;
  String email;
  String language;

  User({
    @required this.id,
    @required this.email,
    this.language,
  });
}

class NoUser extends User {}

1. Problem: Awareness

Users need to be aware of the null object pattern (avoid null), otherwise states rebuilder does not work as expected #153, #159.

2. Problem: Conditional Logic

NoUser is an extension of User and thus the type is also User. The following expectations do not hold and can lead to unforeseen bugs:


// no user is logged in when the app starts (simplifies the example)
final Injected<User> authState = RM.inject(() => NoUser());

void main() {
  test("1. Problem: NoUser is also a User", () {
      expect(authState.state is NoUser, isTrue); // output: true
      expect(authState.state is User, isFalse); //  output: Exception. NoUser is User == true.  
    });
}

If developers make use of the null object pattern, they shall not make conditional logic based on the User class like if (variable is User) {} but only on the null object e.g. if (variables is NoUser){}.

3. Problem: Null object pattern does not "play nice" with null safety

The following code snippet adjusts the User and NoUser class as shown in the first example to null safety:

class User {
  String id;
  String email;
  String? language;

  User({
    required this.id,
    required this.email,
    this.language,
  });
}

class NoUser extends User {
  NoUser() : super(id: "no user", email: "no@user.com");
}

In order to create an extension of the User class, the NoUser class now needs to initialize the non-nullable fields id and email resulting in a very counterintuitive assignation of a mock id and mock email. In other words, the null object pattern now forces giving a non-logged-in/non-existent user a user id.

Proposed Solution

Dart null-safety solves the problem of null values better than the null object pattern. Therefore, allow states to be nullable. In other words, a state defined as the following should be possible (currently it throws an error):

// no user is logged in when the app starts (simplifies the example)
final Injected<User?> authState = RM.inject(() => null);

Injected<User?> makes it very clear that the state could hold no user aka no user is logged in and is good for the following reasons:

  1. Solves the awareness problem by being consistent with Dart syntax.
  2. Solves the conditional logic problem. Dart will force developers to ensure that the user is logged in as the userState.state can be null by either making a null check through if (userState.state != null) or userState.state! if the developer is sure that the user is logged in.
  3. Less verbose than null object pattern.

Concluding Words

States Rebuilder should allow states which are explicitly stated to be nullable e.g. Injected<User?> to be nullable. But besides that, am I missing the point of the null object pattern, or is it redundant and should be removed from the examples/force developers to use it?

GIfatahTH commented 3 years ago

Good point.

With Null object pattern, to check that an object is null, we use 'is'

print(authState.state is NoUser); // print true if state is considered to be null

and to check that an object is not null, we use 'is!' : (=>is not)

print(authState.state is! NoUser); // print true if state is not null

If I set state to be nullable then you have to use null aware operators everywhere in your code.

Imagine how tedious this will be, in your code you have to use (! Or ? ) wherever you refer to the state of the object.

I do not see any problem using the null object pattern here.

What you are doing wrong is you verify that the state is not null by using (is User) when you need to use (is! NoUser).

samuelstroschein commented 3 years ago

Hi @GIfatahTH under this issue too ! :)

I am aware of how to correctly check if a user is logged in or not (after experiencing the "huh why is NoUser a User moment"). That was just one of the arguments why the null object pattern is potentially inferior to "plain" null safety.

Regarding the problem of being forced to use ! or ? everywhere in the app if the type is of Injected<User?>, I don't think it's a big issue since everywhere where the developer accesses the state, a user should be logged in anyways. However, I thought about the following solution (which could probably be abstracted away by injectAuth):

// no user is logged on first opening the app, thus the state is null
final Injected<User?> authState = RM.inject(() -> null, 
    onData: (user) {
      if (user != null) {
          userState = RM.inject(() => user);
          RM.navigate.toAndRemoveUntil(HomeScreen());
       } else {
         RM.navigate.toAndRemoveUntil(LoginScreen());
         userState = null;
     }
  }
)

/// Initialized by authState
///
/// Will never be accessed if no user is logged in, thus
/// the late keyword and avoiding a nullable user. 
late Injected<User> userState;

Maybe there is something I am missing but that solution seems to fix the downside of having to perform null checks or use the exclamation mark !. Even if not, I would prefer using ! to avoid the problems I laid out in the proposal and overall make states rebuilder more beginner friendly.

 

amoslai5128 commented 3 years ago

3. Problem: Null object pattern does not "play nice" with null safety

The following code snippet adjusts the User and NoUser class as shown in the first example to null safety:

class User {
  String id;
  String email;
  String? language;

  User({
    required this.id,
    required this.email,
    this.language,
  });
}

class NoUser extends User {
  NoUser() : super(id: "no user", email: "no@user.com");
}

I agree this problem 3 is sometimes annoying for me. However, I know the reason why States_rebuild designs not to notify when a state is null, it can save for some unwanted rebuild.

I would suggest another solution to let user custom their Injected Interface like:

enum AuthStatus {signed, unSign}  // it can be anything like class or enum

final Injected authState = RM.inject<User?, AuthStatus>(() => null, parameter: AuthStatus.unSign);

class MainScreen extends StatelessWidget {
    Widget build(context) {
        ....
        On.data(()=> Text('Signed out')).listenTo(authState,
            shouldRebuild: ()=> authState.parameter is AuthStatus.unSign,
        ),
}

AuthStatus as a parameter can prevent the users to make unallowed actions. It'd rebuild the widget only when User or AuthStatus is onData. Btw, I think the shouldRebuild: would be nicer to be onlyWhen: or rebuildWhen:

What do u think?

GIfatahTH commented 3 years ago

@samuelstroschein The example you provide wouldn't compile, for two reasons:

  1. userState is a non-nullable object declared using the late keyword. So you are not allowed to set it to null as you do in the example.
  2. Injected objects can either be nullable or non-nullable. If we choose to make it nullable as you suggest, then the userState must be of type Injected<User?>, which returns us back to the null-aware operator.

So far I don't see why non-nullable state is so bad. Or what makes plain null superior to null object pattern.

Please take into consideration that going for nullable state would be considered as a breaking change, and its pros must outweigh its cons to go for it.

GIfatahTH commented 3 years ago

@amoslai5128

I agree this problem 3 is sometimes annoying for me. However, I know the reason why States_rebuild designs no to notify when a state is null, it can save for some unwanted rebuild.

Yes it is a bit annoying.

I would suggest another solution to let user custom their Injected Interface like:

The example you provide is somewhat similar to InjectedAuth.

Btw, I think the shouldRebuild: would be nicer to be onlyWhen: or rebuildWhen:

Good name.

amoslai5128 commented 3 years ago

@GIfatahTH

The example you provide is somewhat similar to InjectedAuth.

Yes, but this would make it more flexible for mutiple use cases, for example:

Before: we need to create to two injected objs.

final activeFilter = RM.inject(() => VisibilityFilter.all);

final Injected<List<Todo>> todosFiltered = RM.inject(
  () {
    if (activeFilter.state == VisibilityFilter.active) {
      return todos.state.where((t) => !t.complete).toList();
    }
    if (activeFilter.state == VisibilityFilter.completed) {
      return todos.state.where((t) => t.complete).toList();
    }
    return [...todos.state];
  },
  ......
);

After:

enum VisibilityFilter {all, active, completed};

final filterTodoRM = RM.Inject<List<Todo>, VisibilityFilter>(() {
    final p = filterTodoRM.parameter;     //Or filterTodoRM.state.parameter
    if (p == VisibilityFilter.active) {
      return todos.state.where((t) => !t.complete).toList();
    }
    if (p == VisibilityFilter.completed) {
      return todos.state.where((t) => t.complete).toList();
    }
    return [...todos.state];
  } ,initParameter: ()=> VisibilityFilter.all );

//It can also like Riverpod, so that we won't get affected by breaking changes
final filterTodoRM = RM.Inject.withParameter<List<Todo>, VisibilityFilter>();  //It sounds natural
final filterTodoRM = RM.ComplexInject<List<Todo>, VisibilityFilter>();
final filterTodoRM = RM.InjectPlus;
final filterTodoRM = RM.InjectWithParameter;
//End options

....
//To change the state's parameter
//Option 1:
filterTodoRM.setState((s, p)=> p = VisibilityFilter.complete);
//Option 2:  
filterTodoRM.setState((s)=> s.parameter = VisibilityFilter.complete);
//Option 3:
filterTodoRM.setState((_)=> filterTodoRM.parameter = VisibilityFilter.complete);
amoslai5128 commented 3 years ago

I'm also thinking should the logic code move away from the UI, like the MVVM pattern, due to the long-run maintainability.

samuelstroschein commented 3 years ago

@samuelstroschein The example you provide wouldn't compile, for two reasons:

  1. userState is a non-nullable object declared using the late keyword. So you are not allowed to set it to null as you do in the example.
  2. Injected objects can either be nullable or non-nullable. If we choose to make it nullable as you suggest, then the userState must be of type Injected<User?>, which returns us back to the null-aware operator.

You are correct. I tested the suggestion with the following code snippet

late var user;

void main(){
  user = "true";
  user= null;
  print(user);
}

which compiles and runs just fine (unexpectedly). Reason being that the type dynamic is excluded from null safety.

So far I don't see why non-nullable state is so bad. Or what makes plain null superior to null object pattern.

It's not necessarily bad but it's coming from Java right? A google search for "dart null object pattern" returns 0 results. That is problematic because Dart developers expect a state to be nullable, especially now with null safety when the developers explicitly states that a state e.g. Injected<User?> can be null. Before null safety, the design decision to not have nullable states already lead to opened issues such as #153 and #159. Now with null safety, the null object pattern gets even more "non-Dart-like" by being forced to define arbitrary non-nullable properties on a null object (pattern) i.e. Problem 3.

I looked at some Provider and Riverpod examples with null safety. They all declare the state to be nullable e.g. User?. I think it all boils down to states_rebuilder being a great package but deviating from Dart syntax likely hurts adoption and leads to unforeseen bugs which lead to lower satisfaction with the package.

Please take into consideration that going for nullable state would be considered as a breaking change, and its pros must outweigh its cons to go for it.

Is it a breaking change though? I assume most developers defined their states non-nullable e.g. Injected<User>. The proposal revolves around allowing explicitly nullable states like Injected<User?> to be null. I did not dig deep into states rebuilders implementation. How would explicitly allowing nullable states break non-nullable states?

samuelstroschein commented 3 years ago

@amoslai5128

class MainScreen extends StatelessWidget {
    Widget build(context) {
        ....
        On.data(()=> Text('Signed out')).listenTo(authState,
            shouldRebuild: ()=> authState.parameter is AuthStatus.unSign,
        ),
}

AuthStatus as a parameter can prevent the users to make unallowed actions. It'd rebuild the widget only when User or AuthStatus is onData. Btw, I think the shouldRebuild: would be nicer to be onlyWhen: or rebuildWhen:

What do u think?

Wouldn't it be easier to just pass the current and next state in shouldRebuild? Under the assumption that a state can be nullable, then the developer could do the following one liner:

final Injected authState = RM.inject<User?>(() => null);

class MainScreen extends StatelessWidget {
    Widget build(context) {
         ....
        On.data(()=> Text('Signed out')).listenTo(authState,
            shouldRebuild: (currentState, nextState) => 
                   currentState.runtimeType != nextState.runtimeType,
      ),
}
GIfatahTH commented 3 years ago

@amoslai5128 I like your proposal.

@samuelstroschein

I did not dig deep into states rebuilders implementation. How would explicitly allowing nullable states break non-nullable states?

I've just completed a full re-implementation of states_rebuilder, I'm going to push it to the dev channel, and at the same time I'm going to create a new branch dev-nullable-state to try with your proposal.

Thank you both for your contribution.

amoslai5128 commented 3 years ago

@GIfatahTH @samuelstroschein

🤔 I also think that we need to improve the structure of the doc, I think this is a very great library that deserves to be more popular.

GIfatahTH commented 3 years ago

@samuelstroschein

I looked at some Provider and Riverpod examples with null safety. They all declare the state to be nullable e.g. User?

I looked at the riverpod and bloc libraries and noticed that they use the "non-nullable" state.

samuelstroschein commented 3 years ago

@GIfatahTH

I looked at the riverpod and bloc libraries and noticed that they use the "non-nullable" state.

Sorry I was a bit unclear. I meant that the User is always (in the examples I saw) nullable. In other words, they don't make use of the null object pattern. But the main argument for nullable states is that Dart developers expect a state/variable to be nullable if it's explicitly allowed to be nullable e.g. User?. I assume Riverpod & CO allow states to be null. I did not test that though.

🤔 I also think that we need to improve the structure of the doc, I think this is a very great library that deserves to be more popular.

Indeed. I assume a dedicated website would help the cause. It's relatively rare to see GitHub Wiki being used and it's a bit clunky to navigate.

GIfatahTH commented 3 years ago

Here is from the auth example of bloc library: https://github.com/felangel/bloc/blob/master/examples/flutter_login/packages/user_repository/lib/src/models/user.dart

class User extends Equatable {
  const User(this.id);

  final String id;

  @override
  List<Object> get props => [id];

  static const empty = User('-'); // This is the null object pattern
}

Notice the empty user that passes a dummy id. This is a common practice in the bloc library. Often people apply patterns without been aware of them.

I have looked throughout riverpod and bloc examples and tests and I haven't seen any nullable state there.

Can you link me to an example using nullable state? I want to learn from their implementations.

samuelstroschein commented 3 years ago

Correct but the code was written before null safety. The user repository class which was updated 27 days ago makes use of a null:

class UserRepository {
  User? _user;

  Future<User?> getUser() async {
    if (_user != null) return _user;
    return Future.delayed(
      const Duration(milliseconds: 300),
      () => _user = User(const Uuid().v4()),
    );
  }
}

That's why I thought the null object pattern is not used anymore (although still in old code).

For Riverpod one of the up-to-date (null safety) examples I found was https://youtu.be/vrPk6LB9bjo?t=345.

When looking at Get(X) I found the following code snipped on the GitHub page:

GetPage redirect( ) {
  final authService = Get.find<AuthService>();
  return authService.authed.value ? null : RouteSettings(name: '/login') // if user is null redirect to login
}

In another repository, I found the following code:

class Root extends GetWidget<AuthController> {
  @override
  Widget build(BuildContext context) {
    return GetX(
      initState: (_) async {
        Get.put<UserController>(UserController());
      },
      builder: (_) {
        if (Get.find<AuthController>().user?.uid != null) {
          return Home();
        } else {
          return Login();
        }
      },
    );
  }
}

Also making use of null.

Unfortunately, most packages don't have a concrete auth example, or have been written before null-safety and are therefore outdated but using a "null pattern" to describe an unsigned user seems common.

GIfatahTH commented 3 years ago

@samuelstroschein Ok, I got it.

I manage to allow the state to be nullable as you suggest. It was a lot easier than I initially thought.

Now you can declare the state with nullable type:

  test(
    'state can be nullable',
    () {
      final counter = RM.inject<int?>(() => null);
      expect(counter.state, null);
      counter.state = 0;
      expect(counter.state, 0);
      counter.state = null;
      expect(counter.state, null);
    },
  );

I refactored the injectedAuth to accept null as an unsigned user.

The distinction between mutable and immutable state is always implicit.

Thanks again for this issue. With your efforts, I'm sure states_rebuilder will be better.

samuelstroschein commented 3 years ago

@GIfatahTH That's great to hear!

Any approximation of when the update will be available?

The distinction between mutable and immutable state is always implicit.

I assume you are talking about #183 right? Why do/did you reject the idea of explicitly defining states as mutable/immutable?

GIfatahTH commented 3 years ago

Any approximation of when the update will be available?

I will push it to the dev channel sooner.

Why do/did you reject the idea of explicitly defining states as mutable/immutable?

1- Because it was my first implementation that I abandoned.

  1. Because it is verbose and so far the implicit deduction of immutability works well.
  2. It's a worthless breaking change.
amoslai5128 commented 3 years ago

@GIfatahTH I also think this can: From

TopAppWidget(
      waiteFor: ()=> [RM.storageInitializer(HiveStorage())]
)

To:

TopAppWidget(
     ensureInitialized: ()=> [WidgetsFlutterBinding.ensureInitialized(), RM.storageInitializer(HiveStorage())],
)
GIfatahTH commented 3 years ago

@amoslai5128

I'm also thinking should the logic code move away from the UI, like the MVVM pattern, due to the long-run maintainability.

Could you explain more. It would be better to open a new issue for this.

TopAppWidget( ensureInitialized: ()=> [WidgetsFlutterBinding.ensureInitialized(), RM.storageInitializer(HiveStorage())], )

good naming.

amoslai5128 commented 3 years ago

TopAppWidget(

@GIfatahTH How cloud I use async in TopAppWidget.ensureInitialization?

GIfatahTH commented 3 years ago

@amoslai5128 Yes you can

ensureInitialization : [
()async{
 //Your async call here
 await initPluggin1();

//Suppose pluggin2 depends on pluggin 1
await initPluggin2();
}(),
]