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

Split GitHub provider out into its own module #1421

Merged
merged 10 commits into from
Aug 29, 2018
Merged

Split GitHub provider out into its own module #1421

merged 10 commits into from
Aug 29, 2018

Conversation

SUPERCILEX
Copy link
Collaborator

Fixes #1389 and #1412

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX SUPERCILEX requested a review from samtstern as a code owner August 27, 2018 05:39
Copy link
Contributor

@samtstern samtstern left a comment

Choose a reason for hiding this comment

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

Mostly LGTM! Just some very small comments.

Also do you think this means we need to go to 5.0.0?

I kinda wanted to save v5 for something cool but I guess I am being overly sentimental about versioning.

@@ -1,4 +1,4 @@
package com.firebase.ui.auth.data.remote;
package com.firebase.ui.auth.github;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be in .github.data.remote just to be consistent?

<string name="facebook_login_protocol_scheme" translatable="false">fbYOUR_APP_ID</string>

<!--
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason to kill these comments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Our documentation in the config.xml is different in the auth vs app module, so I was trying to unify them. I've made it so auth is the documented one with app pointing to it. WDYT? Should it be flipped?

@samtstern samtstern added this to the 4.2.0 milestone Aug 27, 2018
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX
Copy link
Collaborator Author

SUPERCILEX commented Aug 28, 2018

Regarding 5.0, I don't think so? Everything will still compile as before, it'll "just" be a runtime crash. I don't think we need to bump to 5.0 because people have to update their deps anyway to get this change (so it's not super hard to add another dep).

I am being overly sentimental about versioning

🤣

TODO(me):

  • Check release builds
  • Update documentation

Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX SUPERCILEX changed the title [WIP] Split GitHub provider out into its own module Split GitHub provider out into its own module Aug 28, 2018
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
<category android:name="android.intent.category.BROWSABLE" />

<data
android:host="@string/firebase_web_host"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok two questions about this string:

  1. I see we have duplicate strings.xml entries for firebase_web_host in both auth/.../strings.xml and auth-github/.../strings.xml ... is there a reason it needs to exist outside of the GitHub module? I feel like we only use it there but could be forgetting something.
  2. Now that we're sure you want to use GitHub auth when you include the -github module, should we explode at runtime if this string is still equal to CHANGE-ME? I think that might be helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, both those birds were killed with the same stone in the original implementation 😁:

Preconditions.checkConfigured(getApplicationContext(),
"GitHub provider unconfigured. Make sure to add your client id and secret." +
" See the docs for more info:" +
" https://github.com/firebase/FirebaseUI-Android/blob/master/auth/README.md#github",
R.string.firebase_web_host,

  1. We need firebase_web_host in the auth module to do that check
  2. We do that check 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

D'oh

// Needed to override Facebook
implementation(Config.Libs.Support.cardView)
implementation(Config.Libs.Support.customTabs)
implementation(Config.Libs.Support.cardView) // Needed to override Facebook
Copy link
Contributor

Choose a reason for hiding this comment

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

Did something change about how Facebook uses custom tabs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, but we use custom tabs ourselves (for the ToS and PP) so it was a duplicate dependency:

implementation(Config.Libs.Support.customTabs)

@samtstern samtstern merged commit 68be249 into firebase:version-4.2.0-dev Aug 29, 2018
@SUPERCILEX SUPERCILEX deleted the github-split branch August 29, 2018 17:52
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.

2 participants