Template type invokemethod #1365

cyanglaz · 2019-03-20T19:36:51Z

Bump min flutter version to 1.2.0 for most plugins.
Add template type parameter for invokeMethod calls.
Updated invokeMethod and invokeMethod to using invokeListMethod and invokeMapMethod. Reopen the PR.

Anything that will change the type of a public API should not be included in this PR.

Sensor plugin has not been changed since no invokeMethod is called in the plugin.
The plugin version of in app purchase has not be updated because it is still under development.

Need to publish all plugins after this PR is merged, except in_app_purchase and sensor.

flutter/flutter#26431

packages/android_alarm_manager/lib/android_alarm_manager.dart

jonahwilliams · 2019-03-20T19:46:55Z

packages/android_alarm_manager/lib/android_alarm_manager.dart

-    // https://github.com/flutter/flutter/issues/26431
-    // ignore: strong_mode_implicit_dynamic_method
-    final dynamic r = await _channel.invokeMethod('Alarm.periodic', <dynamic>[
+    final dynamic r = await _channel.invokeMethod<dynamic>('Alarm.periodic', <dynamic>[


Suggested change

final dynamic r = await _channel.invokeMethod<dynamic>('Alarm.periodic', <dynamic>[

return _channel.invokeMethod<bool>('Alarm.periodic', <dynamic>[

Co-Authored-By: cyanglaz <[email protected]>

packages/android_alarm_manager/lib/android_alarm_manager.dart

jonahwilliams · 2019-03-20T19:48:50Z

packages/camera/lib/camera.dart

-      // https://github.com/flutter/flutter/issues/26431
-      // ignore: strong_mode_implicit_dynamic_method
-      final Map<dynamic, dynamic> reply = await _channel.invokeMethod(
+      final Map<dynamic, dynamic> reply = await _channel.invokeMethod<Map<dynamic, dynamic>>(


https://github.com/flutter/flutter/blob/5169ab5974571918121fa8a7ddb0d3e0623bc987/packages/flutter/lib/src/services/platform_channel.dart#L329

cool, going to do a massive change on all the invokeMethod that has map type

Co-Authored-By: cyanglaz <[email protected]>

jonahwilliams · 2019-03-20T19:50:02Z

There are many places where you are using <dynamic> and then doing a manual cast - those should be refactored to use <correctType>

cyanglaz · 2019-03-20T19:52:31Z

Closing the PR now for refactoring using invokeListMethod and invokeMapMethod

…ins into template_type_invokemethod

cyanglaz · 2019-03-20T21:33:53Z

Updated invokeMethod and invokeMethod to using invokeListMethod and invokeMapMethod. Reopen the PR.

jonahwilliams · 2019-03-20T21:56:18Z

packages/cloud_functions/lib/cloud_functions.dart

  /// @param functionName The name of the callable function being triggered.
  /// @param parameters Parameters to be passed to the callable function.
  Future<dynamic> call(
      {@required String functionName, Map<String, dynamic> parameters}) async {


I don't know what this is supposed to return, but you could add a similar feature to the invokeMethod to allow users to use inference instead of casting:

Future<T> call( Future<dynamic> call<T>( {@required String functionName, Map<String, dynamic> parameters}) async {

Where if T isn't specified it defaults to dynamic. Example:

final bool x = cloudFunctions.call(functionName: 'hello'); // <T> inferred as bool final y = cloudFunctions.call<String>(functionName: 'bar'); // <T> specified as String

SG, I think we can follow this up in a different PR.
Also, by looking at the implementation, this method should return Future<Map<String, dynamic>>, however it would be breaking change since call is a public method. I would avoid that in this PR. And after this is merged and if we think it is necessary to break, then we can create another PR to fix this.
There are other places that has public API returning which I am also avoiding in this PR.

jonahwilliams

LGTM for invokeMethod, invokeMapMethod

amirh

Still reviewing, sending so I know how far I went so far, will let you know when I got all the way through

amirh · 2019-03-22T21:31:34Z

packages/camera/CHANGELOG.md

+* Bump the minimum flutter version to 1.2.0.
+* Add Template type parameter to `invokeMethod` calls.
+
+


nit: extra empty line here

amirh · 2019-03-22T21:46:36Z

packages/cloud_firestore/CHANGELOG.md

@@ -1,7 +1,7 @@
-## 0.9.7
+## 0.9.6+1


this should be 7+1

amirh · 2019-03-22T22:10:36Z

packages/android_alarm_manager/lib/android_alarm_manager.dart

-  // https://github.com/flutter/flutter/issues/26431
-  // ignore: strong_mode_implicit_dynamic_method
-  _channel.invokeMethod('AlarmService.initialized');
+  _channel.invokeMethod<void>('AlarmService.initialized');


This should be bool following the fix in #1384

amirh · 2019-03-22T22:38:48Z

packages/cloud_firestore/lib/src/write_batch.dart

-      // https://github.com/flutter/flutter/issues/26431
-      // ignore: strong_mode_implicit_dynamic_method
-      : _handle = Firestore.channel.invokeMethod(
+      : _handle = Firestore.channel.invokeMethod<dynamic>(


Isn't the return type for this a int?

amirh · 2019-03-22T22:45:54Z

packages/device_info/lib/device_info.dart

-      isPhysicalDevice: map['isPhysicalDevice'],
-      androidId: map['androidId'],
+      version: AndroidBuildVersion._fromMap(message['version']),
+      board: message['board'],


nit: probably better to keep the rename for a different PR so the blame lines make sense

amirh · 2019-03-22T23:01:11Z

packages/firebase_auth/lib/src/firebase_user.dart

-    // https://github.com/flutter/flutter/issues/26431
-    // ignore: strong_mode_implicit_dynamic_method
-    return await FirebaseAuth.channel.invokeMethod(
+    return await FirebaseAuth.channel.invokeMethod<void>(


This seems to return a Map<String, Object>:

plugins/packages/firebase_auth/android/src/main/java/io/flutter/plugins/firebaseauth/FirebaseAuthPlugin.java

Line 455 in 0403387

currentUser.unlink(provider).addOnCompleteListener(new SignInCompleteListener(result));

amirh · 2019-03-22T23:02:33Z

packages/firebase_auth/pubspec.yaml

 environment:
  sdk: ">=2.0.0-dev.28.0 <3.0.0"
-  flutter: ">=0.1.4 <2.0.0"
+  flutter: ">=1.2.0 <2.0.0"


I got so far

bparrishMines

This PR may be better separated into a PR for each plugin. The use of invokeListMethod and invokeMapMethod seem to assume that the result won't be null and attempts to cast(). If this is true, this PR could lead to a lot of unintended crashes and breaking changes if a user was anticipating a possible null value.

However, they could be changed to only cast if result isn't null. @jonahwilliams.

bparrishMines · 2019-03-25T19:20:24Z

packages/firebase_auth/lib/src/firebase_auth.dart

-    // https://github.com/flutter/flutter/issues/26431
-    // ignore: strong_mode_implicit_dynamic_method
-    final List<dynamic> providers = await channel.invokeMethod(
+    return await channel.invokeListMethod<String>(


Wouldn't invokeListMethod crash when result.cast<T>(); is called and providers == null.

This makes me wonder if we should use invokeListMethod and invokeMapMethod in this PR because in some cases we want to return null values.

bparrishMines · 2019-03-25T19:24:32Z

packages/firebase_dynamic_links/lib/src/firebase_dynamic_links.dart

-        // ignore: strong_mode_implicit_dynamic_method
-        await channel.invokeMethod('FirebaseDynamicLinks#retrieveDynamicLink');
+    final Map<String, dynamic> linkData =
+        await channel.invokeMapMethod<String, dynamic>(


This may crash due to the cast in the method invokeMapMethod call as well. linkData could equal null.

cyanglaz · 2019-03-25T22:15:01Z

I am going to close this one and create different PRs for each plugin. I feel like much safer that way and easier to review.

Chris Yang added 30 commits March 20, 2019 10:12

alarm manager

14d5921

android intent

78b7318

battery

3ff1735

camera

cc16c9d

cloud firestore

5c7ef16

cloud functions

b54a29f

connectivity

efe5b55

git st

30d8e2f

device_info

02c484d

addmob

4c056d6

firebase_anal

530d561

firebase_auth

77b4fc2

firebase_core

3497720

firebase_database

97a295a

dynamic link

a75ab4a

firebase_messaging

bc10d37

ml_vision

a423948

firebase_performance

04b3b31

remote config

88eb149

firebase storage

0ed5fc0

google_maps_flutter

33ca3a4

google sign_in

2947d44

image_picker

ecf894f

in app purchase

a586fe5

local auth

cf8ac32

location background

ebcbdee

ml vision version update

4e060f6

package info

7310313

path provider

ca36d28

quick actions

b52957b

jonahwilliams reviewed Mar 20, 2019

View reviewed changes

packages/android_alarm_manager/lib/android_alarm_manager.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Mar 20, 2019

View reviewed changes

Update packages/android_alarm_manager/lib/android_alarm_manager.dart

c810604

Co-Authored-By: cyanglaz <[email protected]>

jonahwilliams reviewed Mar 20, 2019

View reviewed changes

packages/android_alarm_manager/lib/android_alarm_manager.dart Outdated Show resolved Hide resolved

jonahwilliams reviewed Mar 20, 2019

View reviewed changes

Jonah Williams and others added 2 commits March 20, 2019 12:48

Update packages/android_alarm_manager/lib/android_alarm_manager.dart

9b29dee

Co-Authored-By: cyanglaz <[email protected]>

Update packages/android_alarm_manager/lib/android_alarm_manager.dart

19ec463

Co-Authored-By: cyanglaz <[email protected]>

cyanglaz closed this Mar 20, 2019

Chris Yang and others added 5 commits March 20, 2019 13:02

invokeMapMethod

f2d907c

invokeListMethod

2e77a28

Merge branch 'template_type_invokemethod' of github.com:cyanglaz/plug…

ce58adf

…ins into template_type_invokemethod

remove explict casting

dd4995d

clean up

e92855a

cyanglaz reopened this Mar 20, 2019

more refactors from <dynamic>

92779eb

jonahwilliams reviewed Mar 20, 2019

View reviewed changes

jonahwilliams approved these changes Mar 20, 2019

View reviewed changes

Chris Yang added 2 commits March 20, 2019 15:18

formatting

aa471f1

error fixes

fab684a

amirh mentioned this pull request Mar 22, 2019

Updated android_alarm_manager to work after engine refactor. #642

Merged

amirh reviewed Mar 22, 2019

View reviewed changes

bparrishMines reviewed Mar 25, 2019

View reviewed changes

cyanglaz closed this Mar 25, 2019

julianscheel pushed a commit to jusst-engineering/plugins that referenced this pull request Mar 11, 2020

[firebase_admob] Support v2 embedding (flutter#1365)

68eb9fa

	final dynamic r = await _channel.invokeMethod<dynamic>('Alarm.periodic', <dynamic>[
	return _channel.invokeMethod<bool>('Alarm.periodic', <dynamic>[

		* Bump the minimum flutter version to 1.2.0.
		* Add Template type parameter to `invokeMethod` calls.

Template type invokemethod #1365

Template type invokemethod #1365

Uh oh!

Conversation

cyanglaz commented Mar 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Mar 20, 2019

Uh oh!

cyanglaz commented Mar 20, 2019

Uh oh!

cyanglaz commented Mar 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

amirh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparrishMines left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bparrishMines Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyanglaz commented Mar 25, 2019

Uh oh!

Uh oh!

cyanglaz commented Mar 20, 2019 •

edited

Loading

bparrishMines Mar 25, 2019 •

edited

Loading