[url_launcher] Move away from shared method channel implementation in native packages. #4719

BeMacized · 2022-02-01T14:34:53Z

This PR eliminates the use of the shared method channel implementation in each of the native packages for the url_launcher plugin.

The changes to each native package are pretty much identical to each other. Only interface implementations and tests irrelevant to the respective platform have been removed.

This PR's contents are based on the steps outlined in the following issue:

Eliminate use of shared method channels in flutter/plugins flutter#94224

Pre-launch Checklist

stuartmorgan

Thanks for this cleanup! Mostly looks good, just some minor adjustments.

packages/url_launcher/url_launcher_android/CHANGELOG.md

packages/url_launcher/url_launcher_android/lib/url_launcher_android.dart

packages/url_launcher/url_launcher_android/test/url_launcher_android_test.dart

packages/url_launcher/url_launcher_ios/lib/url_launcher_ios.dart

packages/url_launcher/url_launcher_android/lib/url_launcher_android.dart

packages/url_launcher/url_launcher_ios/test/url_launcher_ios_test.dart

packages/url_launcher/url_launcher_linux/lib/url_launcher_linux.dart

packages/url_launcher/url_launcher_macos/pubspec.yaml

Implement PR Feedback Implement PR Feedback

stuartmorgan

LGTM with one nit (across 5 files)

stuartmorgan · 2022-02-07T20:44:56Z

packages/url_launcher/url_launcher_android/test/url_launcher_android_test.dart

+      return null;
+    });
+
+    final UrlLauncherAndroid launcher = UrlLauncherAndroid();


Minor nit: Please move this into each test. Having the object under test be part of the test fixture makes tests harder to understand, doesn't scale to tests that need to initialize things differently, and makes it much easier to accidentally have state bleed across tests (which I've seen cause tests to pass when they should have failed due to something that was set in a completely different test far more times than I would like).

While this object is so simple it may well never come up, it's an anti-pattern that I don't want to have (or at least, add new) examples of in the codebase. The risk is just not worth saving one statement per test.

stuartmorgan · 2022-02-07T20:47:27Z

packages/url_launcher/url_launcher_ios/test/url_launcher_ios_test.dart

+      return null;
+    });
+
+    final UrlLauncherIOS launcher = UrlLauncherIOS();


stuartmorgan · 2022-02-07T20:47:37Z

packages/url_launcher/url_launcher_linux/test/url_launcher_linux_test.dart

+      return null;
+    });
+
+    final UrlLauncherLinux launcher = UrlLauncherLinux();


stuartmorgan · 2022-02-07T20:47:59Z

packages/url_launcher/url_launcher_macos/test/url_launcher_macos_test.dart

+      return null;
+    });
+
+    final UrlLauncherMacOS launcher = UrlLauncherMacOS();


stuartmorgan · 2022-02-07T20:48:19Z

packages/url_launcher/url_launcher_windows/test/url_launcher_windows_test.dart

+      return null;
+    });
+
+    final UrlLauncherWindows launcher = UrlLauncherWindows();


…ntation in native packages. (flutter/plugins#4719)

ditman · 2022-02-09T00:57:12Z

I think this change has broken the integration test of the core plugin for linux and macos (see this). The error for both is:

MissingPluginException(No implementation found for method canLaunch on channel: plugins.flutter.io/url_launcher)

Stack traces:

MacOS

Running command: "flutter drive -d macos --driver test_driver/integration_test.dart --target integration_test/url_launcher_test.dart" in /private/var/folders/tn/f_9sf1xx5t14qm_6f83q3b840000gn/T/cirrus-ci-build/packages/url_launcher/url_launcher/example
Running "flutter pub get" in example...                          2,141ms
Running pod install...                                           1,059ms
Building macOS application...                                   
flutter: 00:00 +0: canLaunch
flutter: ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
flutter: The following MissingPluginException was thrown running a test:
flutter: MissingPluginException(No implementation found for method canLaunch on channel
flutter: plugins.flutter.io/url_launcher)
flutter: 
flutter: When the exception was thrown, this was the stack:
flutter: #0      MethodChannel._invokeMethod (package:flutter/src/services/platform_channel.dart:165:7)
flutter: <asynchronous suspension>
flutter: #1      canLaunch (package:url_launcher/url_launcher.dart:131:10)
flutter: <asynchronous suspension>
flutter: #2      main.<anonymous closure> (file:///private/var/folders/tn/f_9sf1xx5t14qm_6f83q3b840000gn/T/cirrus-ci-build/packages/url_launcher/url_launcher/example/integration_test/url_launcher_test.dart:16:12)
flutter: <asynchronous suspension>
flutter: #3      testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:170:15)
flutter: <asynchronous suspension>
flutter: #4      TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:830:5)
flutter: <asynchronous suspension>
flutter: 
flutter: The test description was:
flutter:   canLaunch

Linux

Running command: "flutter drive -d linux --driver test_driver/integration_test.dart --target integration_test/url_launcher_test.dart" in /tmp/cirrus-ci-build/packages/url_launcher/url_launcher/example
Running "flutter pub get" in example...                             4.2s
Building Linux application...                                   
flutter: 00:00 +0: canLaunch
flutter: ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
flutter: The following MissingPluginException was thrown running a test:
flutter: MissingPluginException(No implementation found for method canLaunch on channel
flutter: plugins.flutter.io/url_launcher)
flutter: 
flutter: When the exception was thrown, this was the stack:
flutter: #0      MethodChannel._invokeMethod (package:flutter/src/services/platform_channel.dart:165:7)
flutter: <asynchronous suspension>
flutter: #1      canLaunch (package:url_launcher/url_launcher.dart:131:10)
flutter: <asynchronous suspension>
flutter: #2      main.<anonymous closure> (file:///tmp/cirrus-ci-build/packages/url_launcher/url_launcher/example/integration_test/url_launcher_test.dart:16:12)
flutter: <asynchronous suspension>
flutter: #3      testWidgets.<anonymous closure>.<anonymous closure> (package:flutter_test/src/widget_tester.dart:170:15)
flutter: <asynchronous suspension>
flutter: #4      TestWidgetsFlutterBinding._runTestBody (package:flutter_test/src/binding.dart:830:5)
flutter: <asynchronous suspension>
flutter: 
flutter: The test description was:
flutter:   canLaunch

This is the failing test:

https://github.com/flutter/plugins/blob/main/packages/url_launcher/url_launcher/example/integration_test/url_launcher_test.dart

Maybe the build is not picking up the latest versions of the macos/linux plugins? Is the testing app missing some setup?

stuartmorgan · 2022-02-09T01:12:36Z

Hmm, that's the wrong channel name, which suggests that registration of the Dart code isn't working correctly somehow.

ditman · 2022-02-09T01:55:04Z

Is it maybe an issue with flutter drive not initializing the app correctly?

BeMacized · 2022-02-09T14:46:14Z

I'm not quite sure what to do here. I cannot replicate the integration tests failing locally (at least on macos) and as far as I'm aware the checks on the PR itself ran just fine?

Is this an issue with the changes I made or is this an issue with the CI? Should we revert?

stuartmorgan · 2022-02-09T14:51:00Z

It's actually a latent bug in url_launcher itself from the initial addition of the federated packages. Linux and macOS have a typo in the default_package, and that prevents Dart registration from working. I retracted _linux and _macos, and I'm going to do a long-term fix shortly, so there's no need to revert.

stuartmorgan · 2022-02-09T14:55:37Z

as far as I'm aware the checks on the PR itself ran just fine?

We run url_launcher tests against the published versions of the implementation packages. I recently added some analysis with the in-tree state to catch the most common mistakes we make, but this is a runtime failure.

stuartmorgan · 2022-02-09T16:13:26Z

Linux and macOS have a typo in the default_package, and that prevents Dart registration from working. I retracted _linux and _macos

Windows too, which I missed before. Also now retracted.

… implementations (#4777) #4719 converted the url_launcher implementations to Dart-based versions. This should have been completely transparent to clients, but there is a latent bug in `url_launcher` where the `default_package` entries for Linux, macOS, and Windows are incorrect. This went unnoticed until now because currently the only client-facing effect of `default_package` is in resolving which implementation to run Dart registration for. Because of the typos in the `default_package` entries, the resolution in the `flutter` tool doesn't recognize the registration for these packages as needing to run, causing them to be broken at runtime. The affected versions have been retracted to fix the breakage in the short term. This is PR is the first half of the long-term fix; it re-releases them as a major version bump so that existing versions of `url_launcher` will not pick them up. Once they are published, a follow-up PR will update `url_launcher` to fix the typos and relax the version constraints for the dependencies on these implementation packages to allow 2.x or 3.x. (In theory this could be one PR, but doing it as two ensures that we get real testing on the `url_launcher` change, so is safer.) Part of a long-term fix for flutter/flutter#98097

The plugins in the flutter/plugins repo have been moved away from shared method channels in native plugins. In other words, the channel names have been renamed from "url_launcher" to "url_launcher_<platform>". See flutter/plugins#4719 and flutter/flutter#94224 for details.

The plugins in the flutter/plugins repo have been moved away from shared method channels in native plugins. In other words, the channel names have been renamed from `url_launcher` to `url_launcher_<platform>`. See flutter/plugins#4719 and flutter/flutter#94224 for details.

… implementations (#4777) flutter/plugins#4719 converted the url_launcher implementations to Dart-based versions. This should have been completely transparent to clients, but there is a latent bug in `url_launcher` where the `default_package` entries for Linux, macOS, and Windows are incorrect. This went unnoticed until now because currently the only client-facing effect of `default_package` is in resolving which implementation to run Dart registration for. Because of the typos in the `default_package` entries, the resolution in the `flutter` tool doesn't recognize the registration for these packages as needing to run, causing them to be broken at runtime. The affected versions have been retracted to fix the breakage in the short term. This is PR is the first half of the long-term fix; it re-releases them as a major version bump so that existing versions of `url_launcher` will not pick them up. Once they are published, a follow-up PR will update `url_launcher` to fix the typos and relax the version constraints for the dependencies on these implementation packages to allow 2.x or 3.x. (In theory this could be one PR, but doing it as two ensures that we get real testing on the `url_launcher` change, so is safer.) Part of a long-term fix for flutter/flutter#98097

Move away from shared method channel implementation in native packages.

e76b994

github-actions bot added p: url_launcher platform-android platform-ios platform-linux platform-macos platform-windows labels Feb 1, 2022

BeMacized requested a review from stuartmorgan February 2, 2022 12:58

stuartmorgan suggested changes Feb 2, 2022

View reviewed changes

github-actions bot added the platform-web label Feb 3, 2022

BeMacized force-pushed the url_launcher/no_shared_method_channel branch from 736995c to f63ef67 Compare February 3, 2022 15:23

Implement PR Feedback

27753d5

Implement PR Feedback Implement PR Feedback

BeMacized force-pushed the url_launcher/no_shared_method_channel branch from f63ef67 to 27753d5 Compare February 3, 2022 15:25

github-actions bot removed the platform-web label Feb 3, 2022

Fix missed test

19937e5

BeMacized requested a review from stuartmorgan February 3, 2022 15:40

stuartmorgan approved these changes Feb 7, 2022

View reviewed changes

Implement PR feedback

b3aadee

BeMacized merged commit 99feee9 into flutter:main Feb 8, 2022

engine-flutter-autoroll mentioned this pull request Feb 8, 2022

Roll Plugins from c5c8cda753c2 to 99feee9996cb (15 revisions) flutter/flutter#98037

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 8, 2022

99feee999 [url_launcher] Move away from shared method channel impleme…

1bad009

…ntation in native packages. (flutter/plugins#4719)

mvanbeusekom deleted the url_launcher/no_shared_method_channel branch February 9, 2022 16:00

stuartmorgan mentioned this pull request Feb 9, 2022

[url_launcher] Major version bump for desktop Dart-based url_launcher implementations #4777

Merged

11 tasks

jpnurmi mentioned this pull request Feb 14, 2022

Bump url_launcher version & fix tests canonical/ubuntu-desktop-installer#678

Merged

provokateurin mentioned this pull request Feb 21, 2022

url_launcher plugin broken go-flutter-desktop/go-flutter#642

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[url_launcher] Move away from shared method channel implementation in native packages. #4719

[url_launcher] Move away from shared method channel implementation in native packages. #4719

BeMacized commented Feb 1, 2022

stuartmorgan left a comment

stuartmorgan left a comment

stuartmorgan Feb 7, 2022

stuartmorgan Feb 7, 2022

stuartmorgan Feb 7, 2022

stuartmorgan Feb 7, 2022

stuartmorgan Feb 7, 2022

ditman commented Feb 9, 2022 •

edited

stuartmorgan commented Feb 9, 2022

ditman commented Feb 9, 2022 •

edited

BeMacized commented Feb 9, 2022 •

edited

stuartmorgan commented Feb 9, 2022

stuartmorgan commented Feb 9, 2022

stuartmorgan commented Feb 9, 2022

[url_launcher] Move away from shared method channel implementation in native packages. #4719

[url_launcher] Move away from shared method channel implementation in native packages. #4719

Conversation

BeMacized commented Feb 1, 2022

Pre-launch Checklist

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan Feb 7, 2022

Choose a reason for hiding this comment

stuartmorgan Feb 7, 2022

Choose a reason for hiding this comment

stuartmorgan Feb 7, 2022

Choose a reason for hiding this comment

stuartmorgan Feb 7, 2022

Choose a reason for hiding this comment

stuartmorgan Feb 7, 2022

Choose a reason for hiding this comment

ditman commented Feb 9, 2022 • edited

MacOS

Linux

stuartmorgan commented Feb 9, 2022

ditman commented Feb 9, 2022 • edited

BeMacized commented Feb 9, 2022 • edited

stuartmorgan commented Feb 9, 2022

stuartmorgan commented Feb 9, 2022

stuartmorgan commented Feb 9, 2022

ditman commented Feb 9, 2022 •

edited

ditman commented Feb 9, 2022 •

edited

BeMacized commented Feb 9, 2022 •

edited