-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Forbid ... implements UrlLauncherPlatform
#2230
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| ## 1.0.1 | ||
|
|
||
| * Enforce that UrlLauncherPlatform isn't implemented with `implements`. | ||
|
|
||
| ## 1.0.0 | ||
|
|
||
| * Initial release. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,27 +4,48 @@ | |
|
|
||
| import 'dart:async'; | ||
|
|
||
| import 'package:meta/meta.dart' show required; | ||
| import 'package:meta/meta.dart' show required, visibleForTesting; | ||
|
|
||
| import 'method_channel_url_launcher.dart'; | ||
|
|
||
| /// The interface that implementations of url_launcher must implement. | ||
| /// | ||
| /// Platform implementations that live in a separate package should extend this | ||
| /// class rather than implement it as `url_launcher` does not consider newly | ||
| /// added methods to be breaking changes. Extending this class (using `extends`) | ||
| /// ensures that the subclass will get the default implementation, while | ||
| /// platform implementations that `implements` this interface will be broken by | ||
| /// newly added [UrlLauncherPlatform] methods. | ||
| /// Platform implementations should extend this class rather than implement it as `url_launcher` | ||
| /// does not consider newly added methods to be breaking changes. Extending this class | ||
| /// (using `extends`) ensures that the subclass will get the default implementation, while | ||
| /// platform implementations that `implements` this interface will be broken by newly added | ||
| /// [UrlLauncherPlatform] methods. | ||
| abstract class UrlLauncherPlatform { | ||
| /// Only mock implementations should set this to true. | ||
| /// | ||
| /// Mockito mocks are implementing this class with `implements` which is forbidden for anything | ||
| /// other than mocks (see class docs). This property provides a backdoor for mockito mocks to | ||
| /// skip the verification that the class isn't implemented with `implements`. | ||
| @visibleForTesting | ||
| bool get isMock => false; | ||
|
|
||
| /// The default instance of [UrlLauncherPlatform] to use. | ||
| /// | ||
| /// Platform-specific plugins should override this with their own | ||
| /// platform-specific class that extends [UrlLauncherPlatform] when they | ||
| /// register themselves. | ||
| /// | ||
| /// Defaults to [MethodChannelUrlLauncher]. | ||
| static UrlLauncherPlatform instance = MethodChannelUrlLauncher(); | ||
| static UrlLauncherPlatform _instance = MethodChannelUrlLauncher(); | ||
|
|
||
| static UrlLauncherPlatform get instance => _instance; | ||
|
|
||
| static set instance(UrlLauncherPlatform instance) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: style guide gives the order for getters/setters as "getter/field/setter" with no whitespace between them |
||
| if (!instance.isMock) { | ||
| try { | ||
| instance._verifyProvidesDefaultImplementations(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this should be in an assert so it doesn't run in release builds |
||
| } on NoSuchMethodError catch (_) { | ||
| throw AssertionError( | ||
| 'Platform interfaces must not be implemented with `implements`'); | ||
| } | ||
| } | ||
| _instance = instance; | ||
| } | ||
|
|
||
| /// Returns `true` if this platform is able to launch [url]. | ||
| Future<bool> canLaunch(String url) { | ||
|
|
@@ -51,4 +72,12 @@ abstract class UrlLauncherPlatform { | |
| Future<void> closeWebView() { | ||
| throw UnimplementedError('closeWebView() has not been implemented.'); | ||
| } | ||
|
|
||
| // This method makes sure that UrlLauncher isn't implemented with `implements`. | ||
| // | ||
| // See class doc for more details on why implementing this class is forbidden. | ||
| // | ||
| // This private method is called by the instance setter, which fails if the class is | ||
| // implemented with `implements`. | ||
| void _verifyProvidesDefaultImplementations() {} | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,15 +2,40 @@ | |
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| import 'package:mockito/mockito.dart'; | ||
| import 'package:flutter/services.dart'; | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
| import 'package:url_launcher_platform_interface/method_channel_url_launcher.dart'; | ||
| import 'package:url_launcher_platform_interface/url_launcher_platform_interface.dart'; | ||
|
|
||
| import '../lib/method_channel_url_launcher.dart'; | ||
| import '../lib/url_launcher_platform_interface.dart'; | ||
|
|
||
| void main() { | ||
| group('$MethodChannelUrlLauncher', () { | ||
| TestWidgetsFlutterBinding.ensureInitialized(); | ||
| TestWidgetsFlutterBinding.ensureInitialized(); | ||
|
|
||
| group('$UrlLauncherPlatform', () { | ||
| test('$MethodChannelUrlLauncher() is the default instance', () { | ||
| expect(UrlLauncherPlatform.instance, | ||
| isInstanceOf<MethodChannelUrlLauncher>()); | ||
| }); | ||
|
|
||
| test('Cannot be implemented with `implements', () { | ||
| expect(() { | ||
| UrlLauncherPlatform.instance = ImplementsUrlLauncherPlatform(); | ||
| }, throwsA(isInstanceOf<AssertionError>())); | ||
| }); | ||
|
|
||
| test('Can be mocked with `implements', () { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing a backtick here
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| final ImplementsUrlLauncherPlatform mock = ImplementsUrlLauncherPlatform(); | ||
| when(mock.isMock).thenReturn(true); | ||
| UrlLauncherPlatform.instance = mock; | ||
| }); | ||
|
|
||
| test('Can be exteneded', () { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| UrlLauncherPlatform.instance = ExtendsUrlLauncherPlatform(); | ||
| }); | ||
| }); | ||
|
|
||
| group('$MethodChannelUrlLauncher', () { | ||
| const MethodChannel channel = | ||
| MethodChannel('plugins.flutter.io/url_launcher'); | ||
| final List<MethodCall> log = <MethodCall>[]; | ||
|
|
@@ -24,11 +49,6 @@ void main() { | |
| log.clear(); | ||
| }); | ||
|
|
||
| test('is the default $UrlLauncherPlatform instance', () { | ||
| expect(UrlLauncherPlatform.instance, | ||
| isInstanceOf<MethodChannelUrlLauncher>()); | ||
| }); | ||
|
|
||
| test('canLaunch', () async { | ||
| await launcher.canLaunch('http://example.com/'); | ||
| expect( | ||
|
|
@@ -258,3 +278,8 @@ void main() { | |
| }); | ||
| }); | ||
| } | ||
|
|
||
| class ImplementsUrlLauncherPlatform extends Mock | ||
| implements UrlLauncherPlatform {} | ||
|
|
||
| class ExtendsUrlLauncherPlatform extends UrlLauncherPlatform {} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of this functionality will be shared across many plugins. Should it be a mixin or base class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed flutter/flutter#43368 and added a TODO (we will have to wait for a stable release before we can use common framework code for this)