[connectivity_for_web] Fix JS Interop in release mode. #2869
Conversation
…nteracting with the connectivity object.
| // 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'" |
There was a problem hiding this comment.
This was probably Dart telling me that there already existed a native NetworkInformation class, but I misinterpreted as "coming from different parts"
| @@ -38,7 +37,7 @@ class NetworkInformationApiConnectivityPlugin extends ConnectivityPlugin { | |||
| Stream<ConnectivityResult> get onConnectivityChanged { | |||
| if (_connectivityResult == null) { | |||
| _connectivityResult = StreamController<ConnectivityResult>(); | |||
| _networkInformation.onchange = allowInterop((_) { | |||
| _networkInformation.onChange.listen((_) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Interesting! I don't know of any way to detect the app is hot-restarting to clean-up the plugin!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see what you mean, I'm going to create an issue, because this is not exclusive of this plugin!
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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)
* Switch to dart:html window.navigator.connection instead of package:js JS-interop. * Overwrite connection.onchange instead of listening to connection.onChange Stream (prevents multiple subscriptions after hot-reload). * Cleaned up old code related to generating the JS facade.
* Switch to dart:html window.navigator.connection instead of package:js JS-interop. * Overwrite connection.onchange instead of listening to connection.onChange Stream (prevents multiple subscriptions after hot-reload). * Cleaned up old code related to generating the JS facade.
Description
The old version of the plugin used a generated JS-interop file for a class that is available natively from dart:html.
This is not allowed.
Since the native dart:html class is available, this PR changes the plugin to use that (I can't believe I missed that on my previous version!)
This PR also cleans up all the paraphernalia used to generate the old JS-interop file.
Related Issues
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.///).flutter analyze) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?