Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(firebase_remote_config): Firebase Remote Config for Dart and Desktop #71

Draft
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

TimWhiting
Copy link
Collaborator

WIP

Still some more work I can do, but ultimately blocked on installations.

@TimWhiting
Copy link
Collaborator Author

TimWhiting commented May 14, 2022

I have worked around the installations limitation for now by hardcoding an app instance id. It is working in the example app.

It turns out that the firebaseapis does not work as expected. At one point I was getting 404 errors and at one point I was getting auth errors if I remember correctly. Maybe an authenticated client is required? However, the javascript sdk does not use the same api that is generated in firebaseapis, and does not require client authentication other than the project's apiKey. Therefore, I have implemented it in that way to get an initial prototype example working.

Edit: I was looking at the wrong endpoint in the firebaseapis remote config library, and probably was not providing the right parameters in the right format.

@TimWhiting TimWhiting marked this pull request as ready for review May 14, 2022 19:12
pr-Mais added 10 commits May 14, 2022 13:14
 - firebase_auth_dart@0.1.2
 - firebase_auth_desktop@0.1.2
 - firebase_functions_dart@0.1.0+1
 - firebase_functions_desktop@0.1.0+1
 - firebase_auth_dart@0.1.3
 - firebase_auth_desktop@0.1.3
 - firebase_functions_dart@0.1.1
 - firebase_functions_desktop@0.1.0+2
The support for macOS is removed, it's already in official FlutterFire.
 - firebase_auth_desktop@0.2.0
 - firebase_core_desktop@0.2.0
 - firebase_functions_desktop@0.2.0
 - firebase_core_dart@1.0.0
 - firebase_auth_dart@1.0.0
 - firebase_core_desktop@1.0.0
 - firebase_auth_desktop@1.0.0
 - firebase_functions_desktop@0.2.0+1
 - firebase_functions_dart@0.1.1+1
@TimWhiting TimWhiting requested a review from pr-Mais May 14, 2022 19:14
@TimWhiting
Copy link
Collaborator Author

@pr-Mais
While this plugin is not 100% complete, I think it would be good to get a review at this point. The example is working, but there are things like tests and error reporting that aren't 100% complete. I don't know if you'd like to review & merge this & then do the rest in a follow-up PR. Maybe even publish a dev release before the rest is complete?

As installations would be helpful for this and other plugins. I'm willing to work on that next, I know you mentioned that you might work on it, but I don't know if you ever started, and I don't want to replicate work if you are already working on it. It should be a pretty quick one like this.

@TimWhiting TimWhiting marked this pull request as draft May 15, 2022 00:38
@TimWhiting TimWhiting removed the request for review from pr-Mais May 15, 2022 00:38
@TimWhiting
Copy link
Collaborator Author

I just realized I was using the firebaseapis package wrong. I'm going to give that approach another try.

@visibleForTesting
FirebaseRemoteConfig({
required this.app,
this.namespace = 'firebase',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the namespace is used for? couldn't find it in the other plugins

part of '../../firebase_remote_config_dart.dart';

@visibleForTesting
class RemoteConfigApiClient {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Auth, I made sure that the API client class is separate from the package class, so it won't be part of, and the API class on itself is a library. I did it this way cause I wanted to make sure that API internals stay hidden and I don't use its types outside accidentally (in the Desktop package), and control the visibility for package users. Check here, maybe we try to follow the same design?

@pr-Mais
Copy link
Contributor

pr-Mais commented May 16, 2022

Regarding installations, if I start it will open a draft PR immediately, so if you don't see it feel free to start it, I'm currently writing additional tests for Functions.

Once you switch to firebaseapis we can merge right away.

@pr-Mais
Copy link
Contributor

pr-Mais commented May 17, 2022

Hey @TimWhiting, wanna let you know that I started working on Installations and will push the PR soon.

@TimWhiting
Copy link
Collaborator Author

@pr-Mais
I'm running into issues trying to switch this to use the firebaseapis package.

Here is a diff between the two approaches.

https://github.com/TimWhiting/flutterfire_desktop/compare/remote_config...TimWhiting:remote_config_firebaseapis?expand=1

Looking into the firebaseapis it seems as though the generated dart apis use camelCase instead of snake_case for the request body. Additionally the js-sdk (which is where I got the working approach from) puts the apiKey in the queryParameters instead of the request body. I've tried a bunch of variations of the key in the request body, and could not get it to work.

https://github.com/invertase/dart_firebase_apis/blob/c758c22835bd3dde5a3e41c209cc15abcd448b83/generated/firebaseapis/lib/firebaseremoteconfig/v1.dart#L219

The example in the firebaseapis package gets a client from application default credentials which only works in a server environment and is not meant for client side applications from what I think I've read.

@pr-Mais
Copy link
Contributor

pr-Mais commented May 19, 2022

Hmmm.. not sure. Have you tried using clientViaApiKey for the client?

import 'package:googleapis_auth/auth_io.dart';
import 'package:firebaseapis/firebaseremoteconfig/v1.dart' as rc;

_client = client ?? clientViaApiKey(apiConfig.apiKey);
final rcApi =  rc.FirebaseRemoteConfigApi(_client);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants