[firebase_perfomance] iOS side of reformat of Firebase Performance #1556
Conversation
bparrishMines
changed the title
bparrishMines
changed the title
bparrishMines
changed the title
collinjackson
left a comment
collinjackson
left a comment
There was a problem hiding this comment.
Looks like you may still be working on this but wanted to share a few bits of early feedback.
| #import <Flutter/Flutter.h> | ||
|
|
||
| @interface FLTFirebasePerformancePlugin : NSObject <FlutterPlugin> | ||
| + (void)addMethodHandler:(NSNumber *_Nonnull)handle |
There was a problem hiding this comment.
By splitting things into multiple files, you're exposing a lot more class and method surface area here than we were previously. This could become a maintenance burden in the future if people use these APIs and we have to worry about breaking them. I think we should try to make the classes/methods private. Perhaps move addMethodHandler to a file named FirebasePerformancePlugin+Internal.h.
It might also make sense to move FirebasePerformancePlugin.h into a Public/ subfolder, if you can get that to work with the build system, and that will make it more clear that all the other classes are not part of the public API.
See how Firebase CocoaPods are laid out for reference:
https://github.com/firebase/firebase-ios-sdk/tree/master/Functions/FirebaseFunctions
| + (void)registerWithRegistrar:(nonnull NSObject<FlutterPluginRegistrar> *)registrar { | ||
| } | ||
|
|
||
| + (void)sharedInstanceWithCall:(FlutterMethodCall *)call result:(FlutterResult)result { |
There was a problem hiding this comment.
From an Objective-C developer perspective, I feel this name could be confusing. Normally in Objective-C sharedInstance would be an idempotent static method that returns a singleton instance of the class. Here, you're performing a non-idempotent allocation action and returning nothing. Also you're not doing anything with the result argument. Perhaps another name would make it more clear what's going on.
|
|
||
| @interface FLTFirebasePerformancePlugin (Internal) | ||
| + (void)addMethodHandler:(NSNumber *_Nonnull)handle | ||
| methodHandler:(id<FlutterPlugin> _Nonnull)handler; |
There was a problem hiding this comment.
I am not sure how I feel about using id<FlutterPlugin> as the type here. The empty registerWithRegistrar implementations feel like this is a hack that we wouldn't want to propagate to other plugins. These objects aren't really FlutterPlugins, they are proxied classes. It might be a good idea to define a protocol makes this clear, perhaps something we can re-use for other plugins in the future.
There was a problem hiding this comment.
I was thinking the same thing when I was implementing the iOS side. On Android, they are called MethodHandlers and don't have static methods.
I could create a separate protocol such as
@protocol MethodHandler
@required
- (void)handleMethodCall:(FlutterMethodCall*)call result:(FlutterResult)result
@end
@interface FLTTrace : NSObject <MethodHandler>
@end
| tearDownAll(() => completer.complete(null)); | ||
|
|
||
| group('firebase_performance test driver', () { | ||
| // TODO(bparrishMines): Rewrite integration tests when iOS portion is written. |
There was a problem hiding this comment.
Reminder to address this
|
Closing in favor of #1557 |
3 participants
Description
iOS side of possible reformat of Firebase Performance plugin
Related Issues
Android: #1529
Dart: #1530
Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?