MaikuB / flutter_appauth

A Flutter wrapper for AppAuth iOS and Android SDKs
270 stars 243 forks source link

[flutter_appauth] Add const constructor to FlutterAppAuth class #317

Closed shilangyu closed 2 years ago

shilangyu commented 2 years ago

This is useful in scenarios where FlutterAppAuth is being accepted as a parameter, and we want to set a default (defaults have to be const-constructable). Why accept as a parameter? To be able to mock it in tests.

MaikuB commented 2 years ago

Could you elaborate more with your points you raised, ideally with examples in code? The class itself doesn't have any properties to begin with so not sure what you're referring to when it comes to defaults. Furthermore, there are languages out there without const constructors yet it's still possible to mock the class an object is associated with. That is, the ability to mock isn't dependent on being about to have a const constructor and mockito's own example (see https://pub.dev/packages/mockito) shows this

shilangyu commented 2 years ago

Hi! I was a bit too general with my To be able to mock it in tests. statement. This is purely for convenience. Consider the following example:

Let's say I have a class AuthService which uses FlutterAppAuth:

class AuthService {
  const AuthService();

  Future<void> logIn() async {
    final appAuth = FlutterAppAuth();

    // ...
  }
}

Then, I realize I want to mock FlutterAppAuth in tests, so I move it to be a field of my class:

class AuthService {
  AuthService({
    FlutterAppAuth? appAuth,
  }) : appAuth = appAuth ?? FlutterAppAuth();

  final FlutterAppAuth appAuth;

  Future<void> logIn() async {
    // ...
  }
}

Gah! Providing a default implementation is quite cumbersome, and I have lost the const constructor. If FlutterAppAuth was const-constructable:

class AuthService {
  const AuthService({
    this.appAuth = const FlutterAppAuth(),
  });

  final FlutterAppAuth appAuth;

  Future<void> logIn() async {
    // ...
  }
}

Const constructors also have this annoying property that they infect classes using/inheriting them forbidding them from being const-constructable themselves (as seen in the above example). Since, as you said, this is merely a stateless class with some methods on it, it would be beneficial to add a const constructor. I see no downsides to it. Example advantages of having const constructors is not limited to the example above.

not sure what you're referring to when it comes to defaults

Defaults for function parameters

MaikuB commented 2 years ago

Thanks for the explanation and i see what you mean now.

Const constructors also have this annoying property that they infect classes using/inheriting them forbidding them from being const-constructable themselves (as seen in the above example)

I assume you mean constructors that aren't const would causes consuming classes to not be const? If so isn't that more to do with trying to having a method body as you've shown?

Regardless this could be made const but could you resolve the conflicts as I pushed a new release ahead of this to get the example app working again with the new details of the demo IdentityServer instance

shilangyu commented 2 years ago

Rebased

bartekpacia commented 2 years ago

: D