Add Firebase Remote Config support #391

kroikie · 2018-02-27T18:53:42Z

Add plugin to support Firebase Remote Config.

kroikie · 2018-02-27T18:54:49Z

This plugin is incomplete, creating this PR for general design review. Will add tests and clean up formatting once design is finalized.

collinjackson · 2018-03-01T12:42:03Z

packages/firebase_remote_config/lib/remote_config.dart

+  /// Gets the instance of RemoteConfig for the default Firebase app.
+  static RemoteConfig get instance => _instance;
+
+  Future<Map<String, dynamic>> fetch({int expiration: 43200}) async {


Needs dartdoc. It looks like this method activates in addition to fetching, and I'm not sure why that is. can they be separate methods to match the native API?

collinjackson · 2018-03-01T12:44:38Z

packages/firebase_remote_config/lib/remote_config.dart

+  static const MethodChannel _channel =
+      const MethodChannel('firebase_remote_config');
+
+  Map<String, dynamic> _parameters;


I would name this _remoteValues or something

collinjackson · 2018-03-01T12:46:58Z

packages/firebase_remote_config/lib/remote_config.dart

+      const MethodChannel('firebase_remote_config');
+
+  Map<String, dynamic> _parameters;
+  Map<String, dynamic> _defaults;


perhaps _defaultValues to match the above

collinjackson · 2018-03-01T12:48:25Z

packages/firebase_remote_config/lib/remote_config.dart

+  int getInt(String key) {
+    final dynamic value = _parameters[key];
+    if (value != null) {
+      final String strValue = UTF8.decode(value);


This is a bit surprising but it works because when we ask for the value as a byte array on the native side we get a byte array containing a UTF8-encoded stringified int.

collinjackson · 2018-03-19T23:34:18Z

packages/firebase_remote_config/README.md

@@ -0,0 +1,10 @@
+# firebase_remote_config


We should write a real README based on one of the other FlutterFire packages

collinjackson · 2018-03-19T23:34:50Z

...c/main/java/io/flutter/plugins/firebase/firebaseremoteconfig/FirebaseRemoteConfigPlugin.java

+/** FirebaseRemoteConfigPlugin */
+public class FirebaseRemoteConfigPlugin implements MethodCallHandler {
+
+  public static final String TAG = "FirebbaseRCPlugin";


typo, and I would just use the class name probably

collinjackson · 2018-03-19T23:35:33Z

...c/main/java/io/flutter/plugins/firebase/firebaseremoteconfig/FirebaseRemoteConfigPlugin.java

+  private static SharedPreferences sharedPreferences;
+  private final MethodChannel channel;
+
+  /** Plugin registration. */


This comment seems unnecessary

collinjackson · 2018-03-19T23:36:22Z

...c/main/java/io/flutter/plugins/firebase/firebaseremoteconfig/FirebaseRemoteConfigPlugin.java

+        new MethodChannel(registrar.messenger(), "plugins.flutter.io/firebase_remote_config");
+    channel.setMethodCallHandler(new FirebaseRemoteConfigPlugin(channel));
+    sharedPreferences =
+        registrar.context().getSharedPreferences("FirebaseRCPlugin", Context.MODE_PRIVATE);


My preference is to move this string constant into a public static final and use the full class name

collinjackson · 2018-03-19T23:41:11Z

...c/main/java/io/flutter/plugins/firebase/firebaseremoteconfig/FirebaseRemoteConfigPlugin.java

+    return valueMap;
+  }
+
+  private int mapLastFetchStatus(int status) {


It seems like we're not using the same int mappings as iOS or Android, perhaps we should match one or the other.

Thanks they should be. Done.

It looks like these don't match either platform quite yet. After thinking about this some more I think should convert to String instead of int. For example "success" or "no_fetch_yet". It's slightly less efficient but way easier to follow because of the platform differences.

collinjackson · 2018-03-20T00:02:49Z

packages/firebase_remote_config/example/lib/main.dart

+    );
+  }
+
+  Future<void> setupRemoteConfig() async {


I would call this refreshRemoteConfig or something

Happy to rename, but there is a different method that is doing the fetching. This one is used to set the defaults and debug mode, feels like setup stuff. WDYT?

collinjackson · 2018-03-20T00:04:19Z

packages/firebase_remote_config/ios/Classes/FirebaseRemoteConfigPlugin.m

+    resultDict[@"IN_DEBUG_MODE"] =
+        [[NSNumber alloc] initWithBool:[firRemoteConfigSettings isDeveloperModeEnabled]];
+
+    result(resultDict);


I would also put the current remote config values in here

collinjackson · 2018-03-20T00:08:28Z

...c/main/java/io/flutter/plugins/firebase/firebaseremoteconfig/FirebaseRemoteConfigPlugin.java

+                          mapLastFetchStatus(firebaseRemoteConfigInfo.getLastFetchStatus()));
+                      if (!task.isSuccessful()) {
+                        final Exception exception = task.getException();
+                        channel.invokeMethod(


I'm not sure why we are using invokeMethod to return to Dart. It seems like we could return these properties on the method invocation that called "RemoteConfig#fetch". This would make awaiting a fetch() in Dart actually wait for the fetch to happen.

This was used to updated some RemoteConfig properties on the Dart side, however this can be done in handling of the exception so removed this call to invokeMethod.

collinjackson · 2018-03-20T00:10:21Z

packages/firebase_remote_config/lib/src/remote_config.dart

+/// LastFetchStatus defines the possible status values of the last fetch.
+enum LastFetchStatus { success, failure, throttled, noFetchYet }
+
+class FetchThrottledException implements Exception {


My vote is to split each class into a separate file in src/

collinjackson · 2018-03-20T00:14:17Z

packages/firebase_remote_config/lib/src/remote_config.dart

+  RemoteConfig._() {
+    _channel.setMethodCallHandler((MethodCall call) async {
+      switch (call.method) {
+        case 'UpdateFetch':


I think you might be able to remove this

collinjackson · 2018-03-28T21:28:05Z

packages/firebase_remote_config/LICENSE

@@ -0,0 +1 @@
+TODO: Add your license here.


Please add the same license as other plugins

collinjackson · 2018-03-28T21:30:15Z

packages/firebase_remote_config/README.md

+
+Initialize `RemoteConfig`:
+```
+final RemoteConfig _remoteConfig = await RemoteConfig.instance;


I would leave off the _ for this variable since the context is somewhat ambiguous

collinjackson · 2018-03-28T21:31:04Z

packages/firebase_remote_config/README.md

+You can now use the Firebase `_remoteConfig` to fetch remote configurations in your Dart code, e.g.
+```
+final defaults = <String, dynamic>{'welcome': 'default welcome'};
+await _remoteConfig.setDefaults();


I'm not sure you actually need to await here, but you definitely want to pass defaults as an argument

Given that fetch/activate retrieves default values from the native side I think it best to ensure that the setting of defaults completes before doing so. WDYT?

:) forgot the parameter, thanks!

collinjackson · 2018-03-28T21:33:26Z

packages/firebase_remote_config/README.md

+final defaults = <String, dynamic>{'welcome': 'default welcome'};
+await _remoteConfig.setDefaults();
+
+await _remoteConfig.fetch(expiration: new Duration(hours: 5));


maybe const instead of new?

collinjackson · 2018-03-28T21:39:05Z

...c/main/java/io/flutter/plugins/firebase/firebaseremoteconfig/FirebaseRemoteConfigPlugin.java

+      case "RemoteConfig#fetch":
+        {
+          long expiration =
+              call.argument("expiration") instanceof Integer


I would cast call.argument("expiration") as a Number and call its longValue() method

collinjackson · 2018-03-28T22:44:35Z

packages/firebase_remote_config/lib/src/remote_config.dart

+        _parameters[key] = remoteConfigValue;
+      }
+    });
+    return new Future<void>.value();


You should be able to just leave this off I think

collinjackson · 2018-03-28T22:44:54Z

packages/firebase_remote_config/lib/src/remote_config.dart

+      }
+    }
+    print('fetch succeeded');
+    return new Future<void>.value();


You should be able to just leave this off I think

collinjackson · 2018-03-28T22:45:07Z

packages/firebase_remote_config/lib/src/remote_config.dart

+        throw new Exception('Unable to fetch remote config');
+      }
+    }
+    print('fetch succeeded');


We shouldn't be printing here

collinjackson · 2018-03-28T22:45:10Z

packages/firebase_remote_config/lib/src/remote_config.dart

+        final int fetchThrottleEnd = e.details['FETCH_THROTTLED_END'];
+        throw new FetchThrottledException._(endTimeInMills: fetchThrottleEnd);
+      } else {
+        print('fetch failed unknown');


We shouldn't be printing here

collinjackson · 2018-03-28T22:45:13Z

packages/firebase_remote_config/lib/src/remote_config.dart

+          new DateTime.fromMillisecondsSinceEpoch(e.details[lastFetchTimeKey]);
+      _lastFetchStatus = LastFetchStatus.values[e.details[lastFetchStatusKey]];
+      if (e.code == RemoteConfig.fetchFailedThrottledKey) {
+        print('fetch failed throttled');


We shouldn't be printing here

collinjackson · 2018-04-04T00:56:28Z

packages/firebase_remote_config/lib/src/remote_config.dart

+    _remoteConfigSettings = remoteConfigSettings;
+  }
+
+  /// Fetches parameter values for your app. Parameter values may be from


The first sentence of Dartdoc documentation should be a standalone paragraph.

This applies to the other methods in this file as well.

collinjackson · 2018-04-04T00:57:04Z

packages/firebase_remote_config/lib/src/remote_config.dart

+
+  /// Activates the fetched config. This makes fetched key-values take effect.
+  ///
+  /// The returned Future contains true if the fetched config is different


I would say that a Future "completes" to true rather than "contains" true.

collinjackson · 2018-04-04T00:58:41Z

packages/firebase_remote_config/lib/src/remote_config.dart

+    }
+  }
+
+  /// Gets the RemoteConfigValue corresponding to the key. If there is no


Use square brackets for in-scope identifiers like RemoteConfigValue and key.

https://www.dartlang.org/guides/language/effective-dart/documentation#do-use-square-brackets-in-doc-comments-to-refer-to-in-scope-identifiers

collinjackson · 2018-04-04T01:03:52Z

packages/firebase_remote_config/lib/src/remote_config.dart

+      _lastFetchTime =
+          new DateTime.fromMillisecondsSinceEpoch(e.details[_lastFetchTimeKey]);
+      _lastFetchStatus = _parseLastFetchStatus(e.details[_lastFetchStatusKey]);
+      if (e.code == RemoteConfig._fetchFailedThrottledKey) {


I think you can leave off the RemoteConfig. here

collinjackson · 2018-04-04T01:05:45Z

packages/firebase_remote_config/lib/src/remote_config_fetch_throttled_exception.dart

+class FetchThrottledException implements Exception {
+  DateTime _throttleEnd;
+
+  FetchThrottledException._({int endTimeInMills = 43200}) {


I am not sure why there's a default value of 43200 here. It seems like we are already specifying a value in the one place where this exception is constructed, so you can just remove this.

Good point, Done

collinjackson · 2018-04-04T01:13:42Z

packages/firebase_remote_config/lib/src/remote_config.dart

+  RemoteConfigSettings _remoteConfigSettings;
+
+  RemoteConfig._() {
+    _channel.setMethodCallHandler((MethodCall call) async {


This isn't doing anything any more and can probably be removed.

collinjackson · 2018-04-04T01:16:41Z

packages/firebase_remote_config/lib/src/remote_config.dart

+  RemoteConfigSettings get remoteConfigSettings => _remoteConfigSettings;
+
+  /// Gets the instance of RemoteConfig for the default Firebase app.
+  static Future<RemoteConfig> get instance async {


I think we need to make the RemoteConfig instance a singleton stored in a private static member. Otherwise you could have multiple RemoteConfig objects with different default values because they don't know how to synchronize their copies of _parameters.

collinjackson · 2018-04-04T01:29:09Z

packages/firebase_remote_config/ios/Classes/FirebaseRemoteConfigPlugin.m

+    parameterDict[key] = [self createRemoteConfigValueDict:[remoteConfig configValueForKey:key]];
+  }
+  // Add default parameters if missing since `keysWithPrefix` does not return default keys.
+  NSArray *defaultKeys = [[NSUserDefaults standardUserDefaults] arrayForKey:DEFAULT_KEYS];


You can use the allKeysFromSource: API instead of saving these in NSUserDefaults

collinjackson · 2018-04-04T01:29:39Z

packages/firebase_remote_config/ios/Classes/FirebaseRemoteConfigPlugin.m

+
+@implementation FirebaseRemoteConfigPlugin
+
+static NSString *DEFAULT_KEYS = @"default_keys";


I think you can get rid of this and use allKeysFromSource instead

collinjackson · 2018-04-04T01:31:03Z

packages/firebase_remote_config/ios/Classes/FirebaseRemoteConfigPlugin.m

+    FIRRemoteConfig *remoteConfig = [FIRRemoteConfig remoteConfig];
+    NSDictionary *defaults = call.arguments[@"defaults"];
+    [remoteConfig setDefaults:defaults];
+    [[NSUserDefaults standardUserDefaults] setValue:[defaults allKeys] forKey:DEFAULT_KEYS];


This can be removed using allKeysFromSource

collinjackson · 2018-04-23T14:01:04Z

packages/firebase_remote_config/lib/src/remote_config.dart

+/// You can get an instance by calling [RemoteConfig.instance]. Note
+/// [RemoteConfig.instance] is async.
+class RemoteConfig extends ChangeNotifier {
+  static const MethodChannel _channel =


The other plugins make this public and @visibleForTesting to avoid duplicating the channel name in the test, perhaps we should do the same here?

collinjackson · 2018-04-23T14:03:03Z

packages/firebase_remote_config/lib/src/remote_config_value.dart

+
+  RemoteConfigValue._(this._value, this._source);
+
+  ValueSource get source => _source == ValueSource.valueDefault


Add a dartdoc comment here, e.g. "Indicates at which source this value came from."

collinjackson · 2018-04-23T14:09:06Z

packages/firebase_remote_config/lib/src/remote_config.dart

+
+  /// Gets the instance of RemoteConfig for the default Firebase app.
+  static Future<RemoteConfig> get instance async {
+    if (_instance != null) {


There's a race condition here where it's possible to have two instance objects outstanding if you read RemoteConfig.instance twice in a row before the first invokeMethod returns. Consider using a Completer to avoid this. (We should probably have a test to ensure that reading instance twice gives you the same singleton.)

collinjackson · 2018-04-23T14:12:07Z

packages/firebase_remote_config/lib/src/remote_config_settings.dart

+  /// mode is disabled. When developer mode is enabled fetch throttling is
+  /// relaxed to allow many more fetch calls per hour to the remote server than
+  /// the 5 per hour that is enforced when developer mode is disabled.
+  bool debugMode;


This should be final (readonly) to match the native SDKs (e.g. iOS)

collinjackson · 2018-04-23T14:12:23Z

packages/firebase_remote_config/test/firebase_remote_config_test.dart

+    test('setConfigSettings', () async {
+      expect(remoteConfig.remoteConfigSettings.debugMode, true);
+      final RemoteConfigSettings remoteConfigSettings =
+          new RemoteConfigSettings();


Use the constructor to initialize debugMode

collinjackson · 2018-04-23T14:12:43Z

packages/firebase_remote_config/lib/src/remote_config.dart

+    _instance._lastFetchStatus =
+        _parseLastFetchStatus(properties[_lastFetchStatusKey]);
+    final RemoteConfigSettings remoteConfigSettings =
+        new RemoteConfigSettings();


Use the constructor to initialize debugMode

collinjackson · 2018-04-23T14:16:14Z

packages/firebase_remote_config/lib/src/remote_config_settings.dart

+  /// When set to true developer mode is enabled, when set to false developer
+  /// mode is disabled. When developer mode is enabled fetch throttling is
+  /// relaxed to allow many more fetch calls per hour to the remote server than
+  /// the 5 per hour that is enforced when developer mode is disabled.


The first sentence of Dartdoc comments should be its own paragraph.

Consider adding a sentence at the end about what the default is

collinjackson · 2018-04-23T14:23:10Z

packages/firebase_remote_config/lib/src/remote_config.dart

+  static const bool defaultValueForBool = false;
+
+  static const String _fetchFailedThrottledKey = 'FETCH_FAILED_THROTTLED';
+  static const String _lastFetchTimeKey = 'LAST_FETCH_TIME';


I think we should either use constants for all the keys (e.g. 'IN_DEBUG_MODE') or none of them. Also I think the keys should be lower case to match other first-party plugins.

Done, removed constants for keys, made keys lower case.

collinjackson · 2018-04-23T14:25:51Z

packages/firebase_remote_config/ios/firebase_remote_config.podspec

+  s.ios.deployment_target = '6.0'
+  s.dependency 'Flutter'
+  s.dependency 'Firebase/RemoteConfig'
+  s.pod_target_xcconfig = {


Format has changed and this should be static_framework = true now, see 57f8977

collinjackson · 2018-04-23T14:26:30Z

packages/firebase_remote_config/ios/Classes/FirebaseRemoteConfigPlugin.m

+@implementation FirebaseRemoteConfigPlugin
+
+static NSString *LAST_FETCH_TIME_KEY = @"LAST_FETCH_TIME";
+static NSString *LAST_FETCH_STATUS_KEY = @"LAST_FETCH_STATUS";


I would use constants for all of the keys or none of them

Done, removed key constants.

- Used completer to avoid race conditions for config instance - Removed key constants and switched keys to camel case

collinjackson · 2018-04-24T21:18:12Z

packages/firebase_remote_config/lib/src/remote_config.dart

+  static const double defaultValueForDouble = 0.0;
+  static const bool defaultValueForBool = false;
+
+  static RemoteConfig _instance;


You can make this a local variable in _getRemoteConfigInstance() now that you have _instanceCompleter

collinjackson

lgtm once the above comment is addressed

ozexpert · 2018-04-30T14:03:19Z

hi all. when is this coming?

* Add Remote Config support

googlebot added the cla: yes label Feb 27, 2018

collinjackson reviewed Mar 6, 2018

View reviewed changes

collinjackson assigned kroikie Mar 13, 2018

kroikie force-pushed the add-remote-config branch from ea90262 to 54cb972 Compare March 16, 2018 14:55

kroikie assigned collinjackson Mar 16, 2018

collinjackson reviewed Mar 20, 2018

View reviewed changes

kroikie force-pushed the add-remote-config branch from 5480934 to 1dcec0e Compare March 28, 2018 21:09

collinjackson reviewed Mar 28, 2018

View reviewed changes

kroikie force-pushed the add-remote-config branch from f3ca9ac to d8e1c5e Compare April 3, 2018 05:12

collinjackson reviewed Apr 4, 2018

View reviewed changes

collinjackson added feature flutterfire labels Apr 10, 2018

kroikie force-pushed the add-remote-config branch from 6f423df to 8df64fe Compare April 12, 2018 23:07

collinjackson reviewed Apr 23, 2018

View reviewed changes

kroikie added 15 commits April 23, 2018 19:46

Add Remote Config support

887852b

Near full implementation of Remote Config

bd841e6

Full implementation

b18a67a

Fix formatting

56094ce

Fix lint errors

622cf31

Fix formatting again

7406492

Remote project config files

b1b550f

Add config files

4c6866e

Address Comments

1c3aee3

Fix formatting

629398d

Update based on comments

b2862d3

Use native activate result

59ad024

Use strings to pass RC constants

bd8ac30

fix formatting

e8553ec

Fix AdMob formatting

805db80

kroikie added 5 commits April 23, 2018 19:46

fix tests

e50f2eb

Remove unused imports

3343a30

Use static instance

abe19dd

Used consts

83750b2

Update comments

e5e86ca

- Used completer to avoid race conditions for config instance - Removed key constants and switched keys to camel case

kroikie force-pushed the add-remote-config branch from b95f88d to e5e86ca Compare April 24, 2018 02:47

Use tryParse to parse numbers

b8293ff

collinjackson reviewed Apr 24, 2018

View reviewed changes

collinjackson self-requested a review April 27, 2018 18:44

collinjackson approved these changes Apr 27, 2018

View reviewed changes

Remove unnecessary _instance variable

d5d5b35

kroikie merged commit c0e9db5 into flutter:master Apr 30, 2018

haydenflinner pushed a commit to haydenflinner/plugins that referenced this pull request May 4, 2018

Add Firebase Remote Config support (flutter#391)

c0530b6

* Add Remote Config support

slightfoot pushed a commit to slightfoot/plugins that referenced this pull request Jun 5, 2018

Add Firebase Remote Config support (flutter#391)

ae1d26e

* Add Remote Config support

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

Add Firebase Remote Config support (flutter#391)

04aacef

* Add Remote Config support


		@implementation FirebaseRemoteConfigPlugin

		static NSString *DEFAULT_KEYS = @"default_keys";


		RemoteConfigValue._(this._value, this._source);

		ValueSource get source => _source == ValueSource.valueDefault

Add Firebase Remote Config support #391

Add Firebase Remote Config support #391

Conversation

kroikie commented Feb 27, 2018 • edited by collinjackson Loading

kroikie commented Feb 27, 2018

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

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

kroikie Mar 29, 2018 • edited Loading

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

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

collinjackson Apr 23, 2018 • edited Loading

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

collinjackson Apr 23, 2018 • edited Loading

Choose a reason for hiding this comment

kroikie commented Feb 27, 2018 •

edited by collinjackson

Loading

kroikie Mar 29, 2018 •

edited

Loading

collinjackson Apr 23, 2018 •

edited

Loading

collinjackson Apr 23, 2018 •

edited

Loading