-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Support for firebase_core interoperating with native code that configures Firebase apps. #478
Support for firebase_core interoperating with native code that configures Firebase apps. #478
Conversation
…ures Firebase apps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
FirebaseApp app = FirebaseApp.getInstance(name); | ||
result.success(asMap(app)); | ||
} catch (IllegalStateException ex) { | ||
result.success(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might deserve a comment that we successfully return null
here.
} else if ([@"FirebaseApp#appNamed" isEqualToString:call.method]) { | ||
NSString *name = call.arguments; | ||
FIRApp *app = [FIRApp appNamed:name]; | ||
result(app.flutterDictionary); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crash if there is no app with that name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is Objective-C so it just returns nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok. I thought that was only the case with [message passing]
. But perhaps property access is just msg passing under the hood?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
@@ -6,44 +6,71 @@ part of firebase_core; | |||
|
|||
class FirebaseApp { | |||
@visibleForTesting | |||
const FirebaseApp({this.name, @required this.options}); | |||
const FirebaseApp({@required this.name}) : assert(name != null); | |||
|
|||
/// Gets the name of this app. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be /// The name of this app.
per https://www.dartlang.org/guides/language/effective-dart/documentation#prefer-starting-variable-getter-or-setter-comments-with-noun-phrases
|
||
@visibleForTesting | ||
static const MethodChannel channel = const MethodChannel( | ||
'plugins.flutter.io/firebase_core', | ||
); | ||
|
||
static Map<String, FirebaseApp> _namedApps = <String, FirebaseApp>{}; | ||
/// Gets a copy of the options for this app. These are non-modifiable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, getters should be documented with a noun phrase.
}).then((dynamic _) => _namedApps[name]); | ||
await channel.invokeMethod( | ||
'FirebaseApp#configure', | ||
<String, dynamic>{'name': name, 'options': options?.asMap}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
options
cannot be null
here.
); | ||
|
||
setUp(() async { | ||
FirebaseApp.channel | ||
.setMockMethodCallHandler((MethodCall methodCall) async { | ||
log.add(methodCall); | ||
switch (methodCall.method) { | ||
case 'FirebaseApp#appNamed': | ||
if (methodCall.arguments != 'testApp') return null; | ||
return <String, dynamic>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be <dynamic, dynamic>
with production code.
if (methodCall.arguments != 'testApp') return null; | ||
return <String, dynamic>{ | ||
'name': 'testApp', | ||
'options': <String, dynamic>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
'deepLinkURLScheme': 'testDeepLinkURLScheme', | ||
'storageBucket': 'testStorageBucket', | ||
}, | ||
}; | ||
case 'FirebaseApp#allApps': | ||
return <Map<String, dynamic>>[ | ||
<String, dynamic>{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for these containers.
…ures Firebase apps. (flutter#478)
…ures Firebase apps. (flutter#478)
Required for #476
In practice, I doubt anyone would need read the options of an app, it's far more common to set it via
configure()
than read it. So making this APIasync
doesn't affect the developer much.An alternative is that we could make the
options
getter synchronous but throw an exception for the default app unless the developer puts anawait FirebaseApp.configure()
in their code first.