[url_launcher] Add `url_launcher_platform_interface` package #2217

harryterkelsen · 2019-10-18T21:00:36Z

Description

Adds urlLauncherPlatform which can be overridden to implement url_launcher on other platforms (which may not want to use MethodChannels).

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.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
No, this is not a breaking change.

harryterkelsen · 2019-10-21T18:28:03Z

Ping!

amirh · 2019-10-22T00:21:58Z

The platform interface should go in a separate package, otherwise we will have a dependency cycle between url_launcher_web and url_launcher (once the web implementation is endorsed)

harryterkelsen · 2019-10-22T17:28:31Z

OK, then I think this will have to be done in 2 PRs. One to land package:url_launcher_platform_interface and publish it to pub, and then another to migrate package:url_launcher to use it.

harryterkelsen · 2019-10-22T18:08:42Z

PTAL. I moved it to a separate package. Once this lands and we publish it, I will do a follow-up PR to migrate package:url_launcher to actually use it.

ditman

LGTM, but I guess it's Amir's call!

ditman · 2019-10-22T18:57:02Z

packages/url_launcher/url_launcher_platform_interface/pubspec.yaml

+  flutter_test:
+    sdk: flutter
+  url_launcher:
+    path: ../url_launcher


Should this be a dependency on a published version of the plugin, instead of a path?

I think we shouldn't have this dependency here at all (see my comment on the test_

Removed dependency

ditman · 2019-10-22T18:58:03Z

packages/url_launcher/url_launcher_platform_interface/test/url_launcher_platform_test.dart

+  }, skip: true);
+}
+
+class MockUrlLauncherPlatform extends UrlLauncherPlatform {


Maybe extract this to its own test/mocks/mock_url_launcher_platform.dart file?

Removed this test. Just testing the MethodChannel implementation now

amirh · 2019-10-22T18:16:44Z

packages/url_launcher/url_launcher_platform_interface/README.md

+# Usage
+
+To implement a new platform-specific implementation of `url_launcher`, you can
+either implement the method channel calls (specified in


I would not mentioned, I don't think we'll treat that as an officially supported path (e.g we may decide to change the method channel protocol or mechanism at some point).

amirh · 2019-10-22T19:41:02Z

packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart

+  ///
+  /// For documentation on the other arguments, see the `launch` documentation
+  /// in `package:url_launcher/url_launcher.dart`.
+  Future<bool> launch(


These should have default implementations that throw unimplemented.

amirh · 2019-10-22T19:42:15Z

packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart

+import 'dart:async';
+
+/// The interface that implementations of url_launcher must implement.
+abstract class UrlLauncherPlatform {


Add to the class doc that implementers should extend this class and not implement it, otherwise we won't be able to add new methods.

See similar comment on webview:

plugins/packages/webview_flutter/lib/platform_interface.dart

Line 35 in fe9b559

/// Platform implementations that live in a separate package should extend this class rather than

amirh · 2019-10-22T19:45:59Z

packages/url_launcher/url_launcher_platform_interface/test/url_launcher_platform_test.dart

+
+  // TODO(hterkelsen): Re-enable this test once we land the corresponding
+  //     change in package:url_launcher
+  group('UrlLauncher with mock platform', () {


This is testing the app-facing plugin and not the platform interface. Not sure there's anything to test for UrlLauncherPlatform.

I think it will make sense for the tests in this package to be testing the platform channel implementation (with a mock method handler).

Added a test for MethodChannelUrlLauncher

amirh · 2019-10-22T19:49:09Z

packages/url_launcher/url_launcher_platform_interface/lib/url_launcher_platform_interface.dart

+import 'dart:async';
+
+/// The interface that implementations of url_launcher must implement.
+abstract class UrlLauncherPlatform {


The static platform field should be on this class and not on the app-facing plugin (so that platform implementations don't need to depend on the app-facing package)

amirh · 2019-10-22T19:52:08Z

packages/url_launcher/url_launcher_platform_interface/pubspec.yaml

+description: A common platform interface for the url_launcher plugin.
+author: Flutter Team <flutter-dev@googlegroups.com>
+homepage: https://github.com/flutter/plugins/tree/master/packages/url_launcher/url_launcher_platform_interface
+version: 0.1.0


Let's add a comment reminding that is really bad to make a breaking change to this package, and that it's usually better to compromise on a less clean platform interface API. You can refer to the relevant section here: https://docs.google.com/document/d/1LD7QjmzJZLCopUrFAAE98wOUQpjmguyGTN2wd_89Srs/edit#heading=h.xabf9lffxrkb (I'll add a go link for that bookmark)

flutter/website#3142 adds https://flutter.dev/go/platform-interface-breaking-changes

Done (in the README)

amirh · 2019-10-22T19:52:27Z

packages/url_launcher/url_launcher_platform_interface/pubspec.yaml

+description: A common platform interface for the url_launcher plugin.
+author: Flutter Team <flutter-dev@googlegroups.com>
+homepage: https://github.com/flutter/plugins/tree/master/packages/url_launcher/url_launcher_platform_interface
+version: 0.1.0


For that same reason, I'm thinking to start this package at 1.0.0

harryterkelsen · 2019-10-22T21:27:05Z

PTAL. FYI, for the platform interface, I am preferring having all the arguments be required, but maybe it would look nicer if they were @required named arguments instead? Look at the MethodChannelUrlLauncher test to see how it looks with all of the required arguments.

amirh

LGTM! (modulo nit)

amirh · 2019-10-22T21:36:35Z

packages/url_launcher/url_launcher_platform_interface/pubspec.yaml

+description: A common platform interface for the url_launcher plugin.
+author: Flutter Team <flutter-dev@googlegroups.com>
+homepage: https://github.com/flutter/plugins/tree/master/packages/url_launcher/url_launcher_platform_interface
+version: 1.0.0


I'd keep the comment here as well, I feel like ongoing maintainers are less likely to page in the README before publishing, but they will visit this line for sure if they want to bump the major version.

amirh · 2019-10-22T21:42:54Z

PTAL. FYI, for the platform interface, I am preferring having all the arguments be required, but maybe it would look nicer if they were @required named arguments instead? Look at the MethodChannelUrlLauncher test to see how it looks with all of the required arguments.

Good point, I agree it call sites will be less confusing with @required named arguments.

…#2217) * [url_launcher] Add overridable platform * Fix analyzer lints * Run dartfmt * Add a url_launcher_platform_interface package * Disable tests temporarily * Add license header * Fix typo in test * Add LICENSE file * Respond to review comments * Respond to review comments

harryterkelsen added 2 commits October 18, 2019 13:53

[url_launcher] Add overridable platform

a440899

Fix analyzer lints

d1f0ca2

harryterkelsen requested review from collinjackson and amirh October 18, 2019 21:00

googlebot added the cla: yes label Oct 18, 2019

Run dartfmt

d58d524

harryterkelsen added 5 commits October 22, 2019 10:39

Add a url_launcher_platform_interface package

bada3fe

Disable tests temporarily

7cba506

Add license header

3607ed6

Fix typo in test

9c33b61

Add LICENSE file

e6119ee

harryterkelsen changed the title ~~[url_launcher] Add overridable platform~~ [url_launcher] Add url_launcher_platform_interface package Oct 22, 2019

ditman reviewed Oct 22, 2019

View reviewed changes

amirh reviewed Oct 22, 2019

View reviewed changes

collinjackson mentioned this pull request Oct 22, 2019

flutter create --template plugin should make a federated plugin by default flutter/flutter#43284

Open

Respond to review comments

732afcd

harryterkelsen requested review from ditman and amirh October 22, 2019 21:27

amirh approved these changes Oct 22, 2019

View reviewed changes

Respond to review comments

36d1455

harryterkelsen merged commit f04b3c8 into flutter:master Oct 22, 2019

ditman mentioned this pull request Oct 23, 2019

[WIP] Google sign in web ditman/flutter-plugins#85

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[url_launcher] Add `url_launcher_platform_interface` package #2217

[url_launcher] Add `url_launcher_platform_interface` package #2217

harryterkelsen commented Oct 18, 2019

harryterkelsen commented Oct 21, 2019

amirh commented Oct 22, 2019

harryterkelsen commented Oct 22, 2019

harryterkelsen commented Oct 22, 2019

ditman left a comment

ditman Oct 22, 2019

amirh Oct 22, 2019

harryterkelsen Oct 22, 2019

ditman Oct 22, 2019

harryterkelsen Oct 22, 2019

amirh Oct 22, 2019

harryterkelsen Oct 22, 2019

amirh Oct 22, 2019

harryterkelsen Oct 22, 2019

amirh Oct 22, 2019

harryterkelsen Oct 22, 2019

amirh Oct 22, 2019

harryterkelsen Oct 22, 2019

amirh Oct 22, 2019

harryterkelsen Oct 22, 2019

amirh Oct 22, 2019

amirh Oct 22, 2019

harryterkelsen Oct 22, 2019

amirh Oct 22, 2019

harryterkelsen Oct 22, 2019

harryterkelsen commented Oct 22, 2019

amirh left a comment

amirh Oct 22, 2019

harryterkelsen Oct 22, 2019

amirh commented Oct 22, 2019

[url_launcher] Add url_launcher_platform_interface package #2217

[url_launcher] Add url_launcher_platform_interface package #2217

Conversation

harryterkelsen commented Oct 18, 2019

Description

Checklist

Breaking Change

harryterkelsen commented Oct 21, 2019

amirh commented Oct 22, 2019

harryterkelsen commented Oct 22, 2019

harryterkelsen commented Oct 22, 2019

ditman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harryterkelsen commented Oct 22, 2019

amirh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirh commented Oct 22, 2019

[url_launcher] Add `url_launcher_platform_interface` package #2217

[url_launcher] Add `url_launcher_platform_interface` package #2217