Skip to content

Firebase Database Component Registration#286

Merged
schmidt-sebastian merged 1 commit intomasterfrom
mrschmidt-databasecomponents
Mar 16, 2019
Merged

Firebase Database Component Registration#286
schmidt-sebastian merged 1 commit intomasterfrom
mrschmidt-databasecomponents

Conversation

@schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Mar 15, 2019

This is based on what I learned in #277 and adds Component Registration to the RTDB SDK while also removing the usages of FirebaseApp.getToken().

I kept the AndroidAuthTokenProvider, which (when available) wraps the InternalAuthProvider. With this PR, I no longer have access to the RTDB's internal Executor when I initialize the AndroidAuthTokenProvider. To make sure that the callbacks still run on the same executor, I changed the only callsite of getToken() to forward all calls to the existing executor.

The integration tests also no longer initialize a default app, since that is done by our dependencies.

Copy link
Member

@vkryachko vkryachko left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Please see if it makes sense to switch to a lazy dep on auth(e.g. optionalProvider).

@schmidt-sebastian
Copy link
Contributor Author

Re: Optional provider.

I debated doing so, but decided against it. It would change AndroidAuthTokenProvider quite a bit (unless I hide it internally, which doesn't have as much benefit). On top of that, since we follow the same network usage patterns as Firestore, I don't think there is much gain. I suspect that almost all users obtain a database reference and almost immediately connect to the network. I am willing to tackle this in a follow up if we feel that there is opportunity for performance improvements.

@schmidt-sebastian schmidt-sebastian merged commit 5b06f42 into master Mar 16, 2019
@vkryachko vkryachko deleted the mrschmidt-databasecomponents branch March 19, 2019 14:09
@firebase firebase locked and limited conversation to collaborators Oct 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes Override cla size/L

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants