Reforming wifi_iot

[daadu]

Hello everyone 👋

Constant API changes on platforms and increasing functionality has made the current plugin large, complex, and error-prone. Therefore, it would be apt to break the wifi_iot plugin into multiple plugins - for easier management and maintenance. The new plugins I have planned would be as follows:

• 🔴 wifi_basic - check if has WiFi capability; check/enable/disable WiFi service; WiFi informantion (name, rssi/signal strength, ip, submask, broadcast, gateway, etc) [wifi_basic] Checklist for 1.0 #187
• 🟢 wifi_scan - scan WiFi for nearby network [wifi_scan] Checklist for 1.0 #188
• 🔴 wifi_connect_to - connect/disconnect to network [wifi_connect_to] Checklist for 1.0 #189
• 🔴 wifi_ap - setup hotspot [wifi_ap] Checklist for 1.0 #190
• 🔴 wifi_passpoint - discover and connect to Passpoint(Hotspot 2.0) APs [wifi_passpoint] Checklist for 1.0 #290
• 🔴 wifi_aware - join/start WiFi aware network [wifi_aware] Checklist for 1.0 #191
• 🔴 wifi_rtt - request ranging for device positioning [wifi_rtt] Checklist for 1.0 #192
• 🔴 wifi_easy_connect - easy provisiong WiFi credential to peer devices [wifi_easy_connect] Checklist for 1.0 #252

All these plugins would be under the current WiFiFlutter repo. It would use Melos for managing multiple plugins. To keep these plugins in good health, plugin/platform specific code-owner/in-charge could be assigned to oversee the development within that scope.

This measure is necessary to have high-quality WiFi feature coverage with Flutter. Any feedback or criticism on it is welcomed.

[fernando-s97]

Those API use static methods, which are impossible to test AFAIK. Could them be written as instance methods and make the class itself a singleton?

Instead of

final value = Class.method();

We would have

final singletonClass = Class();
final value = singletonClass.method();

This would allow us to work with dependency injection and mock things for testing.

[daadu]

@fernando-s97 Thanks for the suggestion. While I agree with using singleton against static methods - This needs to be communicated to user clearly - that the constuctor/factory call is singleton. So I suggest something like the bellow:

final isWiFiEnabled = await WiFiBasic.instance.isEnabled();

or

final wifiBasic = WiFiBasic.getInstance();
final isWiFiEnabled = await wifiBasic.isEnabled();

Let me know your views on it (I would prefer the 1st approach out of 2) - will it be still an issue for mocking?

Also I am working on wifi_basic implementation at #201 - encourage you to review code there.

[fernando-s97]

@daadu Sorry for the late reply.

This seems to work:

import 'package:flutter_test/flutter_test.dart';
import 'package:mocktail/mocktail.dart';

void main() {
  test('Concrete class', () {
    final WifiBasic wifiBasic = WifiBasic.instance;
    final someClass = SomeClass(wifiBasic);

    final value = someClass.isEnabled;

    expect(value, isTrue);
  });

  test('Manual mock class - true', () {
    final WifiBasic wifiBasic = WifiBasicIsEnabledMock(true);
    final someClass = SomeClass(wifiBasic);

    final value = someClass.isEnabled;

    expect(value, isTrue);
  });

  test('Manual mock class - false', () {
    final WifiBasic wifiBasic = WifiBasicIsEnabledMock(false);
    final someClass = SomeClass(wifiBasic);

    final value = someClass.isEnabled;

    expect(value, isFalse);
  });

  test('Framework mock class - true', () {
    final WifiBasic wifiBasic = WifiBasicMock();
    when(() => wifiBasic.isEnabled()).thenReturn(true);
    final someClass = SomeClass(wifiBasic);

    final value = someClass.isEnabled;

    expect(value, isTrue);
  });

  test('Framework mock class - false', () {
    final WifiBasic wifiBasic = WifiBasicMock();
    when(() => wifiBasic.isEnabled()).thenReturn(false);
    final someClass = SomeClass(wifiBasic);

    final value = someClass.isEnabled;

    expect(value, isFalse);
  });
}

class SomeClass {
  final WifiBasic _wifiBasic;

  SomeClass(this._wifiBasic);

  bool get isEnabled => _wifiBasic.isEnabled();
}

class WifiBasic {
  static final WifiBasic _instance = WifiBasic._();

  static WifiBasic get instance => _instance;

  WifiBasic._();

  bool isEnabled() => true;
}

class WifiBasicIsEnabledMock implements WifiBasic {
  final bool _isEnabled;

  WifiBasicIsEnabledMock(this._isEnabled);

  @override
  bool isEnabled() => _isEnabled;
}

class WifiBasicMock extends Mock implements WifiBasic {}

[daadu]

@fernando-s97 I'm no testing expert, you look like one. I've added some basic test for wifi_scan and wifi_basic in there respective PRs. Can you review them or give your general view on testing a plugin like this. What are your opinions on it.

[fernando-s97]

@daadu I'm not a pro in testing, but I'll take a look (probably on the weekend)

[daadu]

To make sure the functionalities can be extended to multiple platforms in future (web, desktop) and for user to easily manage each use case. I am suggesting an API design approach as follows:

• Each plugin will have no more than 1/2 key function - already achieved by breaking wifi_iot
• Each function should have a corresponding "can/check? method" - for eg for void startScan() -> CanStartScan canStartScan({bool askPermission: true}). As of now the return can be simply an enum - In future can think of having a class with helper method to check specific functionalities - like NotificationSetting in firebase_messaging - this approach could also be reserved for complex "checking scenarios".
• In docs it should be communicated to user to call the "can? method" to check if the functional can be executed - various caveats, etc.
• Apart from that additional utility methods should be provide - to request permission, open settings page, etc. If the user "can" perform but if certain extra steps are required.
• I find this design more easier for user to implement and handle all/some corner cases than throwing exception for missing requirements.

Let me know your views on it.

[fernando-s97]

I'm not sure the lib itself handling permissions is a good thing. I have two views on this:

1. The lib should abstract the permissions part so the user doesn't have to worry about it.
2. The lib should only report errors about missing permissions.

As permission is a more global thing, which can be used in many features, the first approach can impact the UX of the user's application.

Also, about utility methods for permissions, I think we should leave that responsibility to packages that handle this sort of thing, like permission_handler.

So my vote is in favor of the second approach.

[fernando-s97]

Speaking of error handling, I've been working with functional error handling for some time now and I think it's a much better option than exceptions.

So my proposal is to use the dartz package and work with its Either object.

A functional error handling approach would be like:

enum IsWifiEnabledFailure {
    missingPermissionX,
    missingPermissionY,
    unexpectedError
}

Either<IsWifiEnabledFailure, bool> get isWifiEnabled async {
    try {
        final result = (await invokeMethod<bool>('isWifiEnabled'))!;
        return Right(result);
    } on PlatformException (e) {
        switch (e.code) {
            case x:
                return Left(IsWifiEnabledFailure.missingPermissionX);
            case y:
                return Left(IsWifiEnabledFailure.missingPermissionY);
        }
    } catch (e, s) {
        // Log $e and $s
        return Left(IsWifiEnabledFailure.unexpectedError);
    }
}

Future<void> foo() async {
    final Either<IsWifiEnabledFailure, bool> isWifiEnabledResult = await isWifiEnabled;
    final bool? isWifiEnabled = isWifiEnabledResult.fold(
        (failure) {
            late final String errorMessage;
            switch(failure) {
                case IsWifiEnabledFailure.missingPermissionX:
                    errorMessage = 'Missing permission X';
                    break;
                case IsWifiEnabledFailure.missingPermissionY:
                    errorMessage = 'Missing permission Y';
                    break;
                case IsWifiEnabledFailure.unexpectedError:
                    errorMessage = 'Unexpected error';
                    break;
            }

            showSnackbar(errorMessage);

            return null;
        },
        (value) => value,
    );

    if (isWifiEnabled == null) return;

    // Continue the success flow
}

With this approach, we "force" the user to explicitly say what they want to do in case of failure.

[daadu]

While permission are global thing and user should take care of it own its own. But with flutter and its plugins the scenario is different that native development - Here the user when adds a plugin - it is expected that the plugin includes permission handling. This is the reason why almost all plugins have it. While some may choose to handle themselves it is not the majority. Anyways permission handling is optional user can opt-out by passing askForPermission: false - In that case if the permission is required the return is something like noLocationPermissionRequired.

And about throwing exception - I think because it is not part of "method contract" (unlike Jave where void foo() throws XError;) we should not be using it to communicating anything meaningful. Exception should just be errors.

[daadu]

@fernando-s97 Your suggestion of using Either does what we want. But I am afraid it is too opinionated, would required additional dependency and expecting user to know about this pattern. While my suggested approach is more "vanilla" - simply separating the "check" and "execution" into two different methods.

[daadu]

Also since flutter runs on multiple platforms - the permission requirements are different for different platforms - therefore may be user relies on plugin to handle it. To do the same thing - platform X requires A while platform Y requires B. Hope that makes sense.

[fernando-s97]

About the permission, my only concern was about the UX, but I forgot about the askForPermission: false. Since it exists, I think that's fine then.

About the Either proposal, I think this approach is less error prone, but as you said, "would required additional dependency and expecting user to know about this pattern". This alsoconcerns me, and to be honest, I don't have much experience with open source projects and making packages to the crowd, so I'm not the best fit to analyze this kind of impact. I just wanted to put this approach out there.

[daadu]

@fernando-s97 Thanks for your suggestion, I just posted this so that anyone interested in implementing or making future design decision knows why this choice was made.

[daadu]

UPDATE

wifi_scan is ready for review at #205. Request everyone to review, test, give feedback, etc.

If you are using "scan" feature of wifi_iotin your app - try replacing it with it and test.

You can also start with running example app.

For iOS - it should fail silently with appropriate "empty" results - without any additional checks.

Some minor improvements - like implementing toString, == override, etc are still pending - as of now just check if the functions works on various versions of android.

Since, this is the first plugin - near release - this is also a reference (in terms of design, standard, etc) for other plugins to come.

[daadu]

wifi_scan is available in pub.dev now.

[daadu]

Update: added wifi_easy_connect as "additional" protocol to be supported.

[daadu]

Update: re-defining wifi_sta -> wifi_connect_to