[firebase_in_app_messaging] add new plugin #1791

prakhar1989 · 2019-07-02T23:14:56Z

Description

Add a new plugin for Firebase In-App Messaging

Related Issues

None

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.

collinjackson

Thanks for the PR -- this is a great start.

I made some high-level comments on the Dart API. If those sound reasonable let me know when they're addressed and I'll re-review.

It doesn't seem like the integration tests are going to be easy but we should have unit tests at least.

collinjackson · 2019-07-03T00:02:55Z

packages/firebase_inappmessaging/test/firebase_inappmessaging_test.dart

+  const MethodChannel channel = MethodChannel('firebase_inappmessaging');
+
+  setUp(() {
+    channel.setMockMethodCallHandler((MethodCall methodCall) async {


Can you please add unit tests following the pattern of other plugins?

done - let me know what you think!

packages/firebase_inappmessaging/lib/firebase_inappmessaging.dart

filleduchaos · 2019-07-08T17:22:20Z

Oh hey, I was interested in this! Though it kind of slipped through the cracks what with work and all

I think one of the big things people would want from this plugin would be the ability to receive messages in Dart (so they can e.g. be rendered as widgets)? Basically to gain the full range of functionality described in the docs. I am not quite sure what an API that accommodates that would look like though.

collinjackson · 2019-07-08T17:59:57Z

Oh hey, I was interested in this! Though it kind of slipped through the cracks what with work and all

I think one of the big things people would want from this plugin would be the ability to receive messages in Dart (so they can e.g. be rendered as widgets)? Basically to gain the full range of functionality described in the docs. I am not quite sure what an API that accommodates that would look like though.

I agree. It would be great to have that functionality.

prakhar1989 · 2019-07-08T19:10:40Z

Oh hey, I was interested in this! Though it kind of slipped through the cracks what with work and all

I think one of the big things people would want from this plugin would be the ability to receive messages in Dart (so they can e.g. be rendered as widgets)? Basically to gain the full range of functionality described in the docs. I am not quite sure what an API that accommodates that would look like though.

I agree - that'd be a nice feature to have! However, the current In-App Messaging SDK doesn't export the message that's about to be displayed. The only way to customize the messages outside of what the Firebase Console provides would be via forking the SDK as documented here. As this plugin uses the published In-App Messaging SDKs I don't think its possible to provide message customization via Dart Widgets until the underlying SDK supports that functionality.

filleduchaos · 2019-07-10T10:47:36Z

Oh hey, I was interested in this! Though it kind of slipped through the cracks what with work and all
I think one of the big things people would want from this plugin would be the ability to receive messages in Dart (so they can e.g. be rendered as widgets)? Basically to gain the full range of functionality described in the docs. I am not quite sure what an API that accommodates that would look like though.

I agree - that'd be a nice feature to have! However, the current In-App Messaging SDK doesn't export the message that's about to be displayed. The only way to customize the messages outside of what the Firebase Console provides would be via forking the SDK as documented here. As this plugin uses the published In-App Messaging SDKs I don't think its possible to provide message customization via Dart Widgets until the underlying SDK supports that functionality.

Forking the SDK isn't the only way to customize messages - it's pretty much a matter of changing the messageDisplayComponent from the default InAppMessagingDisplay (see Android and iOS). Admittedly that's less straightforward on iOS; the iOS SDK seems pretty strict about having to use the headless flavour if you want to override the message display, but I've never actually tried doing that with the full one.

Fully customizable messages are definitely a nice-to-have though, at this point what I'd say is much more critical is support for event listeners (sending messages to your users isn't very useful when you can't access their response 😅)

collinjackson

Thanks, this is looking good. Sorry for not catching this earlier, but I think you should replace firebase_inappmessaging with firebase_in_app_messaging for consistency with other Flutterfire plugins such remote_config.

collinjackson · 2019-07-10T20:59:58Z

.../android/src/main/java/com/example/firebase_inappmessaging/FirebaseInappmessagingPlugin.java

+import io.flutter.plugin.common.PluginRegistry.Registrar;
+
+/** FirebaseInappmessagingPlugin */
+public class FirebaseInappmessagingPlugin implements MethodCallHandler {


In think this should be FirebaseInAppMessagingPlugin

collinjackson · 2019-07-10T21:00:42Z

.../android/src/main/java/com/example/firebase_inappmessaging/FirebaseInappmessagingPlugin.java

+          result.success(null);
+          break;
+        }
+      case "dataCollectionEnabled":


I would call this setAutomaticDataCollectionEnabled, otherwise it looks like a getter. And "automatic" in the name is consistent across iOS and Android.

Great call - done!

collinjackson · 2019-07-10T21:02:46Z

packages/firebase_inappmessaging/ios/Classes/FirebaseInappmessagingPlugin.m

+
+#import <Firebase/Firebase.h>
+
+@implementation FirebaseInappmessagingPlugin


I would capitalize FirebaseInAppMessagingPlugin

collinjackson · 2019-07-10T21:03:12Z

packages/firebase_inappmessaging/ios/firebase_inappmessaging.podspec

+# To learn more about a Podspec see http://guides.cocoapods.org/syntax/podspec.html
+#
+Pod::Spec.new do |s|
+  s.name             = 'firebase_inappmessaging'


firebase_in_app_messaging

collinjackson · 2019-07-10T21:03:35Z

packages/firebase_inappmessaging/ios/firebase_inappmessaging.podspec

+Pod::Spec.new do |s|
+  s.name             = 'firebase_inappmessaging'
+  s.version          = '0.0.1'
+  s.summary          = 'Firebase InAppMessaging Plugin for Firebase'


This is redundant, maybe "In-app messaging plugin for Firebase"

collinjackson · 2019-07-10T21:04:04Z

packages/firebase_inappmessaging/pubspec.yaml

@@ -0,0 +1,25 @@
+name: firebase_inappmessaging


firebase_in_app_messaging

collinjackson · 2019-07-10T21:04:13Z

packages/firebase_inappmessaging/pubspec.yaml

+
+flutter:
+  plugin:
+    androidPackage: com.example.firebase_inappmessaging


firebase_in_app_messaging

collinjackson · 2019-07-17T17:09:50Z

packages/firebase_in_app_messaging/CHANGELOG.md

@@ -0,0 +1,3 @@
+## 0.0.1
+
+* TODO: Describe initial release.


Suggested change

* TODO: Describe initial release.

* Initial release.

collinjackson · 2019-07-17T17:10:08Z

packages/firebase_in_app_messaging/README.md

@@ -0,0 +1,90 @@
+# firebase_inappmessaging plugin


Suggested change

# firebase_inappmessaging plugin

# firebase_in_app_messaging plugin

collinjackson · 2019-07-17T17:10:23Z

packages/firebase_in_app_messaging/README.md

+
+## Usage
+
+### Import the firebase_inappmessaging plugin


Suggested change

### Import the firebase_inappmessaging plugin

### Import the firebase_in_app_messaging plugin

collinjackson · 2019-07-17T17:10:35Z

packages/firebase_in_app_messaging/README.md

+## Usage
+
+### Import the firebase_inappmessaging plugin
+To use the firebase_inappmessaging plugin, follow the [plugin installation instructions](https://pub.dartlang.org/packages/firebase_inappmessaging#pub-pkg-tab-installing).


Suggested change

To use the firebase_inappmessaging plugin, follow the [plugin installation instructions](https://pub.dartlang.org/packages/firebase_inappmessaging#pub-pkg-tab-installing).

To use the firebase_in_app_messaging plugin, follow the [plugin installation instructions](https://pub.dartlang.org/packages/firebase_inappmessaging#pub-pkg-tab-installing).

collinjackson · 2019-07-17T17:11:08Z

packages/firebase_in_app_messaging/README.md

+
+First off, add the following imports to your Dart code:
+```dart
+import 'package:firebase_inappmessaging/firebase_inappmessaging.dart';


Suggested change

import 'package:firebase_inappmessaging/firebase_inappmessaging.dart';

import 'package:firebase_in_app_messaging/firebase_in_app_messaging.dart';

collinjackson · 2019-07-17T17:17:14Z

packages/firebase_in_app_messaging/test/firebase_in_app_messaging_test.dart

+
+    test('triggerEvent', () async {
+      final FirebaseInAppMessaging fiam = FirebaseInAppMessaging();
+      fiam.triggerEvent('someEvent');


Generally I think we should await async methods in tests to ensure success and prevent race conditions.

collinjackson · 2019-07-17T17:21:48Z

packages/firebase_in_app_messaging/example/lib/main.dart

+            const SizedBox(height: 8),
+            RaisedButton(
+              onPressed: () async {
+                await fiam.triggerEvent('chicken_event');


Is awaiting the event really necessary here? If calling await isn't something we want developers to feel obligated to do, then perhaps we should have onPressed be synchronous here and just fire and forget triggerEvent. There doesn't seem to be any error handling on the native side, so triggerEvent is unlikely to fail, and I think the Future error handling is mostly just for use in tests.

Good call - done!

collinjackson · 2019-07-17T17:27:57Z

packages/firebase_in_app_messaging/example/lib/main.dart

+class MyApp extends StatelessWidget {
+  static FirebaseAnalytics analytics = FirebaseAnalytics();
+  static FirebaseAnalyticsObserver observer =
+      FirebaseAnalyticsObserver(analytics: analytics);


It doesn't seem like you're doing anything with observer here. In order for the observer to be useful you have to pass it to the navigatorObservers of MaterialApp.

Nice catch - removed it.

collinjackson · 2019-07-17T17:30:38Z

packages/firebase_in_app_messaging/example/lib/main.dart

@@ -0,0 +1,122 @@
+import 'dart:async';
+
+import 'package:firebase_analytics/firebase_analytics.dart';


I think we should simplify the example to focus on firebase_in_app_messaging and not include the analytics stuff. To reduce maintenance overhead, plugins should try to avoid unnecessary dependencies on other plugins, even in the examples, unless the other plugins are really necessary to understand how the plugin works. Sometimes it's not possible (e.g. firebase_database, firebase_auth) but here it doesn't seem like there's much interaction going on between analytics and fiam here. Let me know if you feel otherwise...

Hmm, I think having the analytics trigger is a good idea. It's something users ask us about and the example shows doing that in Flutter is equally easy. So I think it's nice to keep since it doesn't add much complexity to the already straightforward example.

OK, we can leave it there for now but we might remove it in a future release.

collinjackson · 2019-07-17T17:31:13Z

packages/firebase_in_app_messaging/README.md

+Make sure to call FirebaseApp.initializeApp(Context) first.
+```
+
+*Note:* When you are debugging on android, use a device or AVD with Google Play services.


Suggested change

*Note:* When you are debugging on android, use a device or AVD with Google Play services.

*Note:* When you are debugging on Android, use a device or AVD with Google Play services.

collinjackson · 2019-07-17T19:28:18Z

packages/firebase_in_app_messaging/example/lib/main.dart

@@ -0,0 +1,122 @@
+import 'dart:async';
+
+import 'package:firebase_analytics/firebase_analytics.dart';


OK, we can leave it there for now but we might remove it in a future release.

* add plugin for firebase inappmessaging

add plugin for firebase inappmessaging

6127d09

googlebot added the cla: yes label Jul 2, 2019

collinjackson self-requested a review July 3, 2019 00:01

collinjackson reviewed Jul 3, 2019

View reviewed changes

prakhar1989 added 2 commits July 2, 2019 19:26

address comments

3c46be0

Fix dart analyze issues

d5ceda4

collinjackson mentioned this pull request Jul 8, 2019

[firebase_in_app_messaging] First draft of a Dart API for in app messaging #1061

Closed

prakhar1989 added 2 commits July 8, 2019 11:07

use instance methods instead of static

a61554b

add unit test for the plugin

37fb5f7

prakhar1989 added 6 commits July 8, 2019 15:10

run dart formatter

da2c34d

fix formatting

82f41f8

fix pubspec.yaml

3a89c02

fix analyze issues

8d810d3

more analyze fixes

2d8f9a9

uhhh formatting

d507113

collinjackson reviewed Jul 10, 2019

View reviewed changes

collinjackson added in review flutterfire labels Jul 11, 2019

prakhar1989 added 5 commits July 16, 2019 11:41

s/firebase_inappmessaging/firebase_in_app_messaging

ee0ea7a

change method name to setAutomaticDataCollectionEnabled

7c82b7f

reformatting

1bce955

add test_driver and change call arguments

64eb8a7

remove useless comments

1a769fc

collinjackson changed the title ~~[firebase_inappmessaging] add new plugin~~ [firebase_in_app_messaging] add new plugin Jul 17, 2019

collinjackson reviewed Jul 17, 2019

View reviewed changes

collinjackson self-assigned this Jul 17, 2019

action feedback

d71ff60

collinjackson approved these changes Jul 17, 2019

View reviewed changes

clean up readme in example project

00fee03

collinjackson merged commit 990bba9 into flutter:master Jul 18, 2019

prakhar1989 deleted the inappmessaging-plugin branch July 18, 2019 22:57

mithun-mondal pushed a commit to bKash-developer/archived_plugins that referenced this pull request Aug 6, 2019

[firebase_in_app_messaging] add new plugin (flutter#1791)

71ad08c

* add plugin for firebase inappmessaging


		#import <Firebase/Firebase.h>

		@implementation FirebaseInappmessagingPlugin

	# firebase_inappmessaging plugin
	# firebase_in_app_messaging plugin

	### Import the firebase_inappmessaging plugin
	### Import the firebase_in_app_messaging plugin

	To use the firebase_inappmessaging plugin, follow the [plugin installation instructions](https://pub.dartlang.org/packages/firebase_inappmessaging#pub-pkg-tab-installing).
	To use the firebase_in_app_messaging plugin, follow the [plugin installation instructions](https://pub.dartlang.org/packages/firebase_inappmessaging#pub-pkg-tab-installing).

	import 'package:firebase_inappmessaging/firebase_inappmessaging.dart';
	import 'package:firebase_in_app_messaging/firebase_in_app_messaging.dart';

		@@ -0,0 +1,122 @@
		import 'dart:async';

		import 'package:firebase_analytics/firebase_analytics.dart';

	Note: When you are debugging on android, use a device or AVD with Google Play services.
	Note: When you are debugging on Android, use a device or AVD with Google Play services.

[firebase_in_app_messaging] add new plugin #1791

[firebase_in_app_messaging] add new plugin #1791

Uh oh!

Conversation

prakhar1989 commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Checklist

Breaking Change

Uh oh!

collinjackson 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

filleduchaos commented Jul 8, 2019

Uh oh!

collinjackson commented Jul 8, 2019

Uh oh!

prakhar1989 commented Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filleduchaos commented Jul 10, 2019

Uh oh!

collinjackson 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!

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!

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!

prakhar1989 commented Jul 2, 2019 •

edited

Loading

prakhar1989 commented Jul 8, 2019 •

edited

Loading