-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Open
Labels
Needs AttentionThis issue needs maintainer attention.This issue needs maintainer attention.platform: allIssues / PRs which are for all platforms.Issues / PRs which are for all platforms.plugin: coretype: bugSomething isn't workingSomething isn't workingtype: enhancementNew feature or requestNew feature or request
Description
Bug report
FirebasePluginPlatform
contains dead code. It should be renamed to FirebasePlugin
and should not extend PlatformInterface
.
Reasoning:
FirebasePluginPlatform.verifyExtends
is not ever called. There is no reason for FirebasePluginPlatform
to be a PlatformInterface
. Having it extend PlatformInterface
is fragile because if a method were ever added to FirebasePluginPlatform
or PlatformInterface
, it could break compatibility with libraries that are using these classes with implements
.
Steps to reproduce
class CustomFirebaseAppCheck implements FirebaseAppCheck {
CustomFirebaseAppCheck() { ... }
@override
final FirebaseApp app;
@override
Future<void> activate({String? webRecaptchaSiteKey}) { ... }
@override
Future<String?> getToken([bool? forceRefresh]) { ... }
@override
Future<void> setTokenAutoRefreshEnabled(bool isTokenAutoRefreshEnabled) { ... }
@override
Stream<String?> get onTokenChange { ... }
}
Everything would work initially, but minor updates such as new methods added to FirebasePluginPlatform
would cause the plugin to fail to compile.
Expected behavior
- Remove
FirebasePluginPlatform.verifyExtends
, which is dead code. FirebasePluginPlatform
should not extendPlatformInterface
. Subclasses ofPlatformInterface
are not part of the app-facing plugin interface.- Rename
FirebasePluginPlatform
toFirebasePlugin
. (Optionally, provide a soft-deprecated compatibility shimFirebasePluginPlatform
that extendsFirebasePlugin
so that this isn't a breaking change.) Update every plugin's app-facing class to extend this newFirebasePlugin
class. This is to make it clear that the class is exposed to apps, unlike platform classes.
Additional context
Metadata
Metadata
Assignees
Labels
Needs AttentionThis issue needs maintainer attention.This issue needs maintainer attention.platform: allIssues / PRs which are for all platforms.Issues / PRs which are for all platforms.plugin: coretype: bugSomething isn't workingSomething isn't workingtype: enhancementNew feature or requestNew feature or request