refactor!: rework remote config plugin #4186

kroikie · 2020-11-23T02:46:08Z

Description

Update remote config plugin to:

Add proper testing
Use platform interface

Related Issues

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. Updating the pubspec.yaml and changelogs is not required.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
All existing and new tests are passing.
I updated/added relevant documentation (doc comments with ///).
The analyzer (flutter analyze) does not report any problems on my PR.
I read and followed the Flutter Style Guide.
I signed the CLA.
I am willing to follow-up on review comments in a timely manner.

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.

Ehesp

Looking good so far 👍 Few comments, however main things once it's ready are:

Error handling around the Method Channel calls so errors are passed as FirebaseExceptions (example)
Pub code comments

packages/firebase_remote_config/firebase_remote_config/lib/src/remote_config.dart

..._config_platform_interface/lib/src/method_channel/method_channel_firebase_remote_config.dart

IchordeDionysos

OMG thanks, I really like that a Remote config rework is coming soon 😍

packages/firebase_remote_config/firebase_remote_config/lib/src/remote_config.dart

IchordeDionysos · 2020-12-17T13:50:40Z

This would also make this PR obsolete #2061.

Fixes #195

..._remote_config/firebase_remote_config_platform_interface/lib/src/remote_config_settings.dart

kroikie · 2020-12-28T23:38:01Z

@Ehesp you mentioned here that we should convert platform exceptions to Firebase exceptions. Do you think this should be done:

a: on all calls over the channel
b: on all dev visible calls over the channel
c: on all calls over the channel that can return an exception

WDYT?

Ehesp · 2020-12-29T08:09:59Z

@kroikie generally we do this when an error can be thrown from a user side-effect, so when there are calls over the bridge from a direct user action.

Ehesp

Looking good - few things commented on. @Salakar / @russellwheatley could you give the iOS a look over?

packages/firebase_remote_config/firebase_remote_config/analysis_options.yaml

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

packages/firebase_remote_config/firebase_remote_config/lib/src/remote_config.dart

..._config_platform_interface/lib/src/method_channel/method_channel_firebase_remote_config.dart

...config/firebase_remote_config_platform_interface/lib/src/method_channel/utils/exception.dart

packages/firebase_remote_config/firebase_remote_config/lib/src/remote_config.dart

packages/firebase_remote_config/firebase_remote_config/example/lib/main.dart

packages/firebase_remote_config/firebase_remote_config/lib/src/remote_config.dart

ncuillery

In iOS, the example app crashes when fetching the config in airplane mode:

Stacktrace

2021-01-05 10:51:10.872634-0500 Runner[94569:3476689] Task <C1792643-7196-4EE5-BB54-0D9EC5DDF6EC>.<1> HTTP load failed, 0/0 bytes (error code: -1009 [1:50])
2021-01-05 10:51:10.877075-0500 Runner[94569:3476689] Task <C1792643-7196-4EE5-BB54-0D9EC5DDF6EC>.<1> finished with error [-1009] Error Domain=NSURLErrorDomain Code=-1009 "The Internet connection appears to be offline." UserInfo={_kCFStreamErrorCodeKey=50, NSUnderlyingError=0x6000026e79c0 {Error Domain=kCFErrorDomainCFNetwork Code=-1009 "(null)" UserInfo={_kCFStreamErrorCodeKey=50, _kCFStreamErrorDomainKey=1}}, _NSURLErrorFailingURLSessionTaskErrorKey=LocalDataTask <C1792643-7196-4EE5-BB54-0D9EC5DDF6EC>.<1>, _NSURLErrorRelatedURLSessionTaskErrorKey=(
    "LocalDataTask <C1792643-7196-4EE5-BB54-0D9EC5DDF6EC>.<1>"
), NSLocalizedDescription=The Internet connection appears to be offline., NSErrorFailingURLStringKey=https://firebaseinstallations.googleapis.com/v1/projects/flutterfire-cd2f7/installations/, NSErrorFailingURLKey=https://firebaseinstallations.googleapis.com/v1/projects/flutterfire-cd2f7/installations/, _kCFStreamErrorDomainKey=1}
2021-01-05 10:51:10.878082-0500 Runner[94569:3476281] *** Assertion failure in -[FlutterError initWithCode:message:details:], ../../flutter/shell/platform/darwin/common/framework/Source/FlutterChannels.mm:105
2021-01-05 10:51:10.878134-0500 Runner[94569:3476477] 6.33.0 - [Firebase/RemoteConfig][I-RCN000073] Failed to get installations token. Error : Error Domain=com.firebase.installations Code=0 "(null)" UserInfo={NSUnderlyingError=0x6000026e0570 {Error Domain=NSURLErrorDomain Code=-1009 "The Internet connection appears to be offline." UserInfo={_kCFStreamErrorCodeKey=50, NSUnderlyingError=0x6000026e79c0 {Error Domain=kCFErrorDomainCFNetwork Code=-1009 "(null)" UserInfo={_kCFStreamErrorCodeKey=50, _kCFStreamErrorDomainKey=1}}, _NSURLErrorFailingURLSessionTaskErrorKey=LocalDataTask <C1792643-7196-4EE5-BB54-0D9EC5DDF6EC>.<1>, _NSURLErrorRelatedURLSessionTaskErrorKey=(
    "LocalDataTask <C1792643-7196-4EE5-BB54-0D9EC5DDF6EC>.<1>"
), NSLocalizedDescription=The Internet connection appears to be offline., NSErrorFailingURLStringKey=https://firebaseinstallations.googleapis.com/v1/projects/flutterfire-cd2f7/installations/, NSErrorFailingURLKey=https://firebaseinstallations.googleapis.com/v1/projects/flutterfire-cd2f7/installations/, _kCFStreamErrorDomainKey=1}}}.
2021-01-05 10:51:10.914839-0500 Runner[94569:3476281] *** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Code cannot be nil'
*** First throw call stack:
(
	0   CoreFoundation                      0x00007fff20420af6 __exceptionPreprocess + 242
	1   libobjc.A.dylib                     0x00007fff20177e78 objc_exception_throw + 48
	2   CoreFoundation                      0x00007fff2042091f +[NSException raise:format:] + 0
	3   Foundation                          0x00007fff2077056a -[NSAssertionHandler handleFailureInMethod:object:file:lineNumber:description:] + 191
	4   Flutter                             0x00000001089da9b7 -[FlutterError initWithCode:message:details:] + 244
	5   Flutter                             0x00000001089da8b1 +[FlutterError errorWithCode:message:details:] + 56
	6   Runner                              0x0000000107e47c6d +[FLTFirebasePlugin createFlutterErrorFromCode:message:optionalDetails:andOptionalNSError:] + 653
	7   Runner                              0x0000000107e49777 __57-[FLTFirebaseRemoteConfigPlugin handleMethodCall:result:]_block_invoke + 391
	8   Runner                              0x0000000107e4a5a9 __71-[FLTFirebaseRemoteConfigPlugin fetchAndActivate:withMethodCallResult:]_block_invoke + 121
	9   Runner                              0x0000000107def81c __57-[FIRRemoteConfig fetchAndActivateWithCompletionHandler:]_block_invoke + 332
	10  Runner                              0x0000000107e033b8 __65-[RCNConfigFetch reportCompletionOnHandler:withStatus:withError:]_block_invoke + 40
	11  libdispatch.dylib                   0x000000010a4117ec _dispatch_call_block_and_release + 12
	12  libdispatch.dylib                   0x000000010a4129c8 _dispatch_client_callout + 8
	13  libdispatch.dylib                   0x000000010a420e75 _dispatch_main_queue_callback_4CF + 1152
	14  CoreFoundation                      0x00007fff2038edbb __CFRUNLOOP_IS_SERVICING_THE_MAIN_DISPATCH_QUEUE__ + 9
	15  CoreFoundation                      0x00007fff2038963e __CFRunLoopRun + 2685
	16  CoreFoundation                      0x00007fff203886d6 CFRunLoopRunSpecific + 567
	17  GraphicsServices                    0x00007fff2bededb3 GSEventRunModal + 139
	18  UIKitCore                           0x00007fff24690e0b -[UIApplication _run] + 912
	19  UIKitCore                           0x00007fff24695cbc UIApplicationMain + 101
	20  Runner                              0x0000000107dc3ec0 main + 112
	21  libdyld.dylib                       0x00007fff202593e9 start + 1
)
libc++abi.dylib: terminating with uncaught exception of type NSException
*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Code cannot be nil'
CoreSimulator 732.18.6 - Device: iPhone 11 (3AE4D40C-3A69-46F3-98C0-695EA67BF824) - Runtime: iOS 14.3 (18C61) - DeviceType: iPhone 11
terminating with uncaught exception of type NSException

To repro:

Launch the example app on iOS device or simulator
Turn to airplane mode (or turn off the laptop wifi if using the simulator)
Tap on the fetch button

google-cla · 2021-01-05T19:01:10Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

github-actions · 2021-01-12T17:20:22Z

Visit the preview URL for this PR (updated for commit b146368):

https://flutter-firebase-docs--pr4186-remote-config-rework-i0k40rvu.web.app

_{(expires Tue, 19 Jan 2021 19:54:17 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

packages/firebase_remote_config/firebase_remote_config_platform_interface/README.md

…m_interface/README.md Co-authored-by: Mike Diarmid <[email protected]>

IchordeDionysos · 2021-01-12T18:41:22Z

@kroikie what are your plans with this PR? Do you plan to merge this PR with the reworked remote config API and in a second PR introduce support for the web.
Or do you plan to put web support in this PR as well?

Basically, my question is when you intend to merge/release the reworked plugin?

google-cla · 2021-01-12T18:46:54Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

kroikie · 2021-01-12T18:47:37Z

@IchordeDionysos the plan will be to land this update with the Platform Interface support then add web support in a follow up PR.

…terface on example app

google-cla · 2021-01-12T18:50:20Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

Salakar · 2021-01-12T19:00:21Z

@googlebot I consent.

docs/remote-config/usage.mdx

Co-authored-by: Mike Diarmid <[email protected]>

ened · 2021-01-15T19:03:19Z

...es/firebase_remote_config/firebase_remote_config/ios/Classes/FLTFirebaseRemoteConfigPlugin.m

+NSString *const kFirebaseRemoteConfigChannelName = @"plugins.flutter.io/firebase_remote_config";
+
+@interface FLTFirebaseRemoteConfigPlugin ()
+@property(nonatomic, retain) FlutterMethodChannel *channel;


this field is unused & could be removed.

It only seems to be set to nil in dealloc

@kroikie @rrousselGit

ened · 2021-01-15T19:04:06Z

...es/firebase_remote_config/firebase_remote_config/ios/Classes/FLTFirebaseRemoteConfigPlugin.m

+  }
+}
+
+- (void)detachFromEngineForRegistrar:(NSObject<FlutterPluginRegistrar> *)registrar {


This will never be called because the plugin is not calling publish, per the API docs:

You will only receive this method if you registered your plugin instance with the `FlutterEngine` via `-[FlutterPluginRegistry publish:]`.

kroikie requested review from Ehesp and Salakar as code owners November 23, 2020 02:46

google-cla bot added the cla: yes label Nov 23, 2020

kroikie marked this pull request as draft November 23, 2020 18:45

Ehesp requested changes Dec 16, 2020

View reviewed changes

packages/firebase_remote_config/firebase_remote_config/lib/src/remote_config.dart Outdated Show resolved Hide resolved

..._config_platform_interface/lib/src/method_channel/method_channel_firebase_remote_config.dart Outdated Show resolved Hide resolved

IchordeDionysos reviewed Dec 17, 2020

View reviewed changes

packages/firebase_remote_config/firebase_remote_config/lib/src/remote_config.dart Outdated Show resolved Hide resolved

IchordeDionysos reviewed Dec 18, 2020

View reviewed changes

..._remote_config/firebase_remote_config_platform_interface/lib/src/remote_config_settings.dart Outdated Show resolved Hide resolved

kroikie force-pushed the remote-config-rework branch from 80a21cc to 0eee980 Compare December 28, 2020 23:43

kroikie marked this pull request as ready for review December 28, 2020 23:43

kroikie changed the title ~~[WIP] rework remote config plugin~~ rework remote config plugin Dec 28, 2020

kroikie changed the title ~~rework remote config plugin~~ refactor: rework remote config plugin Dec 29, 2020

This was referenced Dec 29, 2020

[firebase_remote_config] Remote config/double instance error #2061

Closed

Bad state: Future already completed #195

Closed

kroikie changed the title ~~refactor: rework remote config plugin~~ refactor[firebase_remote_config]: rework remote config plugin Dec 30, 2020

kroikie changed the title ~~refactor[firebase_remote_config]: rework remote config plugin~~ refactor: rework remote config plugin Dec 30, 2020

kroikie added the plugin: remote_config label Dec 30, 2020

Ehesp requested changes Dec 31, 2020

View reviewed changes

Ehesp reviewed Jan 1, 2021

View reviewed changes

packages/firebase_remote_config/firebase_remote_config/lib/src/remote_config.dart Show resolved Hide resolved

rrousselGit reviewed Jan 4, 2021

View reviewed changes

packages/firebase_remote_config/firebase_remote_config/example/lib/main.dart Show resolved Hide resolved

rrousselGit reviewed Jan 4, 2021

View reviewed changes

packages/firebase_remote_config/firebase_remote_config/lib/src/remote_config.dart Outdated Show resolved Hide resolved

rrousselGit reviewed Jan 4, 2021

View reviewed changes

packages/firebase_remote_config/firebase_remote_config/lib/src/remote_config.dart Show resolved Hide resolved

rrousselGit reviewed Jan 4, 2021

View reviewed changes

packages/firebase_remote_config/firebase_remote_config/lib/src/remote_config.dart Outdated Show resolved Hide resolved

ncuillery reviewed Jan 5, 2021

View reviewed changes

kroikie force-pushed the remote-config-rework branch from a596b9f to 0c7d24f Compare January 5, 2021 19:00

google-cla bot added cla: no and removed cla: yes labels Jan 5, 2021

kroikie added 2 commits January 12, 2021 09:17

docs: Update changelog and migration guide

1efbc0d

chore: bump verion of firebase_remote_config plugin

6e52739

kroikie force-pushed the remote-config-rework branch from e2927a1 to 6e52739 Compare January 12, 2021 17:18

chore: bump version of firebase_core_platform_interface

d901577

Salakar changed the title ~~refactor: rework remote config plugin~~ refactor!: rework remote config plugin Jan 12, 2021

Salakar reviewed Jan 12, 2021

View reviewed changes

packages/firebase_remote_config/firebase_remote_config_platform_interface/README.md Outdated Show resolved Hide resolved

Update packages/firebase_remote_config/firebase_remote_config_platfor…

bbaa03a

…m_interface/README.md Co-authored-by: Mike Diarmid <[email protected]>

Salakar added 2 commits January 12, 2021 18:43

docs: add usage sidebar link

1082675

docs: update plugin readme

d2b319c

google-cla bot added cla: no and removed cla: yes labels Jan 12, 2021

chore: add dependency_override for firebase_remote_config_platform_in…

cfd37e2

…terface on example app

google-cla bot added cla: yes and removed cla: no labels Jan 12, 2021

Salakar and others added 3 commits January 12, 2021 19:03

chore(example): commit Podfile with osx 10.12 deployment target

8cc394b

fix(macos): add missing symlinks

65f537a

docs: update usage docs

7cb628f

Salakar reviewed Jan 12, 2021

View reviewed changes

docs/remote-config/usage.mdx Outdated Show resolved Hide resolved

Update docs/remote-config/usage.mdx

b146368

Co-authored-by: Mike Diarmid <[email protected]>

Salakar approved these changes Jan 13, 2021

View reviewed changes

Salakar merged commit 20ab6c9 into master Jan 14, 2021

Salakar deleted the remote-config-rework branch January 14, 2021 00:23

ened reviewed Jan 15, 2021

View reviewed changes

firebase locked and limited conversation to collaborators Feb 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: rework remote config plugin #4186

refactor!: rework remote config plugin #4186

kroikie commented Nov 23, 2020 •

edited by Salakar

Loading

Ehesp left a comment

IchordeDionysos left a comment

IchordeDionysos commented Dec 17, 2020

kroikie commented Dec 28, 2020

Ehesp commented Dec 29, 2020

Ehesp left a comment

ncuillery left a comment

google-cla bot commented Jan 5, 2021

github-actions bot commented Jan 12, 2021 •

edited

Loading

IchordeDionysos commented Jan 12, 2021

google-cla bot commented Jan 12, 2021

kroikie commented Jan 12, 2021

google-cla bot commented Jan 12, 2021

Salakar commented Jan 12, 2021

ened Jan 15, 2021

ened Jan 15, 2021

refactor!: rework remote config plugin #4186

refactor!: rework remote config plugin #4186

Conversation

kroikie commented Nov 23, 2020 • edited by Salakar Loading

Description

Related Issues

Checklist

Breaking Change

Ehesp left a comment

Choose a reason for hiding this comment

IchordeDionysos left a comment

Choose a reason for hiding this comment

IchordeDionysos commented Dec 17, 2020

kroikie commented Dec 28, 2020

Ehesp commented Dec 29, 2020

Ehesp left a comment

Choose a reason for hiding this comment

ncuillery left a comment

Choose a reason for hiding this comment

google-cla bot commented Jan 5, 2021

github-actions bot commented Jan 12, 2021 • edited Loading

IchordeDionysos commented Jan 12, 2021

google-cla bot commented Jan 12, 2021

kroikie commented Jan 12, 2021

google-cla bot commented Jan 12, 2021

Salakar commented Jan 12, 2021

ened Jan 15, 2021

Choose a reason for hiding this comment

ened Jan 15, 2021

Choose a reason for hiding this comment

kroikie commented Nov 23, 2020 •

edited by Salakar

Loading

github-actions bot commented Jan 12, 2021 •

edited

Loading