Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[connectivity_for_web] Fix JS Interop in release mode. #2869

Merged
ditman merged 8 commits into from
Jul 13, 2020
Merged

[connectivity_for_web] Fix JS Interop in release mode. #2869

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2cb3a21
Remove auto-generator script, since we don't use almost anything of i…
ditman Jul 10, 2020
6bfadf4
Switch to getProperty instead of accessing properties directly when i…
ditman Jul 10, 2020
43c8d71
Migrate to dart:html NetworkInformation API.
ditman Jul 10, 2020
6429ff5
Bump version
ditman Jul 10, 2020
3e38a71
Make package publishable again.
ditman Jul 10, 2020
dbb76d5
Make version check happy
ditman Jul 10, 2020
3069f31
Overwrite onchange until we can free resources on hot restart.
ditman Jul 13, 2020
5dde17d
Make analyzer happy
ditman Jul 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/connectivity/connectivity_for_web/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.3.1

* Use NetworkInformation API from dart:html, instead of the JS-interop version.

## 0.3.0

* Rename from "experimental_connectivity_web" to "connectivity_for_web", and move to flutter/plugins master.
Expand Down
78 changes: 0 additions & 78 deletions packages/connectivity/connectivity_for_web/lib/src/generated/network_information_types.dart
View file
Open in desktop

This file was deleted.

13 changes: 6 additions & 7 deletions ...onnectivity/connectivity_for_web/lib/src/network_information_api_connectivity_plugin.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,28 +1,27 @@
import 'dart:async';
import 'dart:html' as html show window, NetworkInformation;

import 'package:connectivity_platform_interface/connectivity_platform_interface.dart';
import 'package:connectivity_for_web/connectivity_for_web.dart';
import 'package:flutter/foundation.dart';
import 'package:js/js.dart';

import 'generated/network_information_types.dart' as dom;
import 'utils/connectivity_result.dart';

/// The web implementation of the ConnectivityPlatform of the Connectivity plugin.
class NetworkInformationApiConnectivityPlugin extends ConnectivityPlugin {
final dom.NetworkInformation _networkInformation;
final html.NetworkInformation _networkInformation;

/// A check to determine if this version of the plugin can be used.
static bool isSupported() => dom.navigator?.connection != null;
static bool isSupported() => html.window.navigator.connection != null;

/// The constructor of the plugin.
NetworkInformationApiConnectivityPlugin()
: this.withConnection(dom.navigator?.connection);
: this.withConnection(html.window.navigator.connection);

/// Creates the plugin, with an override of the NetworkInformation object.
@visibleForTesting
NetworkInformationApiConnectivityPlugin.withConnection(
dom.NetworkInformation connection)
html.NetworkInformation connection)
: _networkInformation = connection;

/// Checks the connection status of the device.
Expand All @@ -38,7 +37,7 @@ class NetworkInformationApiConnectivityPlugin extends ConnectivityPlugin {
Stream<ConnectivityResult> get onConnectivityChanged {
if (_connectivityResult == null) {
_connectivityResult = StreamController<ConnectivityResult>();
_networkInformation.onchange = allowInterop((_) {
_networkInformation.onChange.listen((_) {
Copy link
Contributor

@mdebbar mdebbar Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is unrelated to your PR, but I noticed it and wanted to point it out. The listener being attached here is never cleaned up. This will potentially cause problems with hot restart since the old listener will remain intact while a new listener is created.

Copy link
Contributor

@mdebbar mdebbar Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, before this PR there used to be only one listener that you keep overriding. Now you're using a stream that could have multiple listeners which makes it possible to leak listeners.

Copy link
Member Author

@ditman ditman Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! I don't know of any way to detect the app is hot-restarting to clean-up the plugin!

Copy link
Contributor

@mdebbar mdebbar Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is how we do it in the web engine. But that's only one example that requires clean up, there may be others.

One thing you can do is make the listening lazy. So you only attach a listener when someone subscribes to _connectivityResult.stream. And as soon as there are no more subscribers to _connectivityResult.stream you can release your listener. Doing it this way ensures that your code won't leak anything as long as the user code isn't leaking the stream subscription.

Copy link
Member Author

@ditman ditman Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean, I'm going to create an issue, because this is not exclusive of this plugin!

Copy link
Member Author

@ditman ditman Jul 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are indeed listeners leaking. Two problems:

  • The hot restart never calls "dispose" on the main app, so the client never detaches from the stream (this can be worked around on the app side)
  • Even if I manually cancel the subscription to the outermost Stream, the innermost stream is never canceled (this is the bug that you were mentioning)

I'm going to fix the current asymmetry: subscribe to the inner Stream only when there's a new listener, and unsubscribe from it when there are none left.

Thanks for bringing this up!

Copy link
Member Author

@ditman ditman Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created a Dart issue regarding the first issue: dart-lang/sdk#42679

Without knowing when to call "dispose", we can't clear up the Stream.

I've reverted that part of the code to write directly to the "onchange" DOM property of the object, until we get a clear way of cleaning up after ourselves (from the plugin)

_connectivityResult
.add(networkInformationToConnectivityResult(_networkInformation));
});
Expand Down
17 changes: 6 additions & 11 deletions packages/connectivity/connectivity_for_web/lib/src/utils/connectivity_result.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,22 @@
import 'dart:html' as html show NetworkInformation;
import 'package:connectivity_platform_interface/connectivity_platform_interface.dart';

/// Converts an incoming NetworkInformation object into the correct ConnectivityResult.
//
// We can't be more specific on the signature of this method because the API is odd,
// data can come from a static value in the DOM, or as the 'target' of a DOM Event.
//
// If we type info as `NetworkInformation`, Dart will complain with:
// "Uncaught Error: Expected a value of type 'NetworkInformation',
// but got one of type 'NetworkInformation'"
Comment on lines -5 to -10
Copy link
Member Author

@ditman ditman Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was probably Dart telling me that there already existed a native NetworkInformation class, but I misinterpreted as "coming from different parts"

ConnectivityResult networkInformationToConnectivityResult(
dynamic /* NetworkInformation */ info) {
html.NetworkInformation info,
) {
if (info == null) {
return ConnectivityResult.none;
}
if (info.downlink == 0 && info.rtt == 0) {
return ConnectivityResult.none;
}
if (info.type != null) {
return _typeToConnectivityResult(info.type);
}
if (info.effectiveType != null) {
return _effectiveTypeToConnectivityResult(info.effectiveType);
}
if (info.type != null) {
return _typeToConnectivityResult(info.type);
}
return ConnectivityResult.none;
}

Expand Down
4 changes: 1 addition & 3 deletions packages/connectivity/connectivity_for_web/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: connectivity_for_web
description: An implementation for the web platform of the Flutter `connectivity` plugin. This uses the NetworkInformation Web API, with a fallback to Navigator.onLine.
version: 0.3.0
version: 0.3.1
homepage: https://github.com/ditman/plugins/tree/connectivity-web/packages/connectivity/experimental_connectivity_web

flutter:
Expand All @@ -12,7 +12,6 @@ flutter:

dependencies:
connectivity_platform_interface: ^1.0.3
js: ^0.6.1+1
flutter_web_plugins:
sdk: flutter
flutter:
Expand All @@ -24,7 +23,6 @@ dev_dependencies:
sdk: flutter
flutter_test:
sdk: flutter
e2e: ^0.2.4+3

environment:
sdk: ">=2.6.0 <3.0.0"
Expand Down
21 changes: 12 additions & 9 deletions packages/connectivity/connectivity_for_web/test/lib/main.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import 'package:flutter_test/flutter_test.dart';
import 'package:connectivity_platform_interface/connectivity_platform_interface.dart';
import 'package:connectivity_for_web/src/network_information_api_connectivity_plugin.dart';

import 'package:mockito/mockito.dart';

import 'src/connectivity_mocks.dart';

void main() {
Expand All @@ -16,11 +18,12 @@ void main() {
num rtt = 50,
ConnectivityResult expected,
}) {
MockNetworkInformation connection = MockNetworkInformation(
type: type,
effectiveType: effectiveType,
downlink: downlink,
rtt: rtt);
final connection = MockNetworkInformation();
when(connection.type).thenReturn(type);
when(connection.effectiveType).thenReturn(effectiveType);
when(connection.downlink).thenReturn(downlink);
when(connection.rtt).thenReturn(downlink);

NetworkInformationApiConnectivityPlugin plugin =
NetworkInformationApiConnectivityPlugin.withConnection(connection);
expect(plugin.checkConnectivity(), completion(equals(expected)));
Expand Down Expand Up @@ -53,16 +56,16 @@ void main() {

group('get onConnectivityChanged', () {
test('puts change events in a Stream', () async {
MockNetworkInformation connection =
MockNetworkInformation(effectiveType: '4g', downlink: 10, rtt: 50);
final connection = MockNetworkInformation();
NetworkInformationApiConnectivityPlugin plugin =
NetworkInformationApiConnectivityPlugin.withConnection(connection);

Stream<ConnectivityResult> results = plugin.onConnectivityChanged;

// Fake a disconnect-reconnect
connection.mockChangeValue(downlink: 0, rtt: 0);
connection.mockChangeValue(downlink: 10, rtt: 50);
await connection.mockChangeValue(downlink: 0, rtt: 0);
await connection.mockChangeValue(
downlink: 10, rtt: 50, effectiveType: '4g');

// The stream of results is infinite, so we need to .take(2) for this test to complete.
expect(
Expand Down
54 changes: 11 additions & 43 deletions packages/connectivity/connectivity_for_web/test/lib/src/connectivity_mocks.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,60 +1,28 @@
import 'dart:async';
import 'dart:html';

import 'package:connectivity_for_web/src/generated/network_information_types.dart'
as dom;
import 'package:mockito/mockito.dart';

/// A Mock implementation of the NetworkInformation API that allows
/// for external modification of its values.
class MockNetworkInformation extends dom.NetworkInformation {
@override
String type;

@override
String effectiveType;

@override
num downlink;
class MockNetworkInformation extends Mock implements NetworkInformation {
StreamController<Event> _onChangeController = StreamController<Event>();

@override
num rtt;

@override
EventListener onchange;

/// Constructor of mocked instances...
MockNetworkInformation({
this.type,
this.effectiveType,
this.downlink,
this.rtt,
});
Stream<Event> get onChange => _onChangeController.stream;

/// Changes the desired values, and triggers the change event listener.
void mockChangeValue({
String type,
String effectiveType,
num downlink,
num rtt,
}) {
this.type = type ?? this.type;
this.effectiveType = effectiveType ?? this.effectiveType;
this.downlink = downlink ?? this.downlink;
this.rtt = rtt ?? this.rtt;
}) async {
when(this.type).thenAnswer((_) => type);
when(this.effectiveType).thenAnswer((_) => effectiveType);
when(this.downlink).thenAnswer((_) => downlink);
when(this.rtt).thenAnswer((_) => rtt);

onchange(Event('change'));
_onChangeController.add(Event('change'));
}

@override
void addEventListener(String type, listener, [bool useCapture]) {}

@override
bool dispatchEvent(Event event) {
return true;
}

@override
Events get on => null;

@override
void removeEventListener(String type, listener, [bool useCapture]) {}
}
1 change: 1 addition & 0 deletions packages/connectivity/connectivity_for_web/test/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ dev_dependencies:
flutter_driver:
sdk: flutter
e2e: ^0.2.4+3
mockito: ^4.1.1

environment:
sdk: ">=2.6.0 <3.0.0"
Expand Down
2 changes: 0 additions & 2 deletions packages/connectivity/connectivity_for_web/ts/.gitignore
View file
Open in desktop

This file was deleted.

25 changes: 0 additions & 25 deletions packages/connectivity/connectivity_for_web/ts/README.md
View file
Open in desktop

This file was deleted.

Loading