Skip to content

Auth instances cleanup #786

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

Merged
merged 15 commits into from
Jul 11, 2017
Merged

Auth instances cleanup #786

merged 15 commits into from
Jul 11, 2017

Conversation

SUPERCILEX
Copy link
Collaborator

@samtstern While working on the anonymous auth PR, it was frustrating to have to pass in flow params each time I wanted to get an auth or user instance. This PR fixes that issue with a slightly hacky solution of storing the flow params in a static variable that's reset each time the helper activity is created.

This PR is built on top of #785 which is built on top of #784.

SUPERCILEX added 11 commits July 2, 2017 15:54
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
…nstances-cleanup

# Conflicts:
#	auth/src/main/java/com/firebase/ui/auth/provider/FacebookProvider.java
#	auth/src/main/java/com/firebase/ui/auth/provider/TwitterProvider.java
#	auth/src/main/java/com/firebase/ui/auth/ui/email/RegisterEmailFragment.java
#	auth/src/main/java/com/firebase/ui/auth/ui/phone/CustomCountDownTimer.java
#	auth/src/main/java/com/firebase/ui/auth/ui/phone/PhoneVerificationActivity.java
Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
@samtstern
Copy link
Contributor

This change strikes me as a little strange. We've avoided, until now, keeping FlowParams as static. Instead each Activity and Fragment has kept the flow params as a member variable and passed it along through the flow.

If we do this change, then we'd have a mixed approach. A more consistent thing to do would be to make this change and then add a static FlowParameters getFlowParams() method to AuthInstances but I don't really like that idea since static variables are generally code smell.

@SUPERCILEX
Copy link
Collaborator Author

@samtstern Yeah, upon looking back at this PR it does seem kinda gross. I've updated it to convert the AuthInstances into an AuthHelper object. While I know you were trying to get rid of the helpers, I feel like keeping a simple and contained helper just for the auth stuff makes sense and reduces the bloat of having to pass in those FlowParams each time. What do you think?

Signed-off-by: Alex Saveau <[email protected]>
Signed-off-by: Alex Saveau <[email protected]>
@SUPERCILEX
Copy link
Collaborator Author

@samtstern Well that was rough. Finally got the build to pass though! 😄

Signed-off-by: Alex Saveau <[email protected]>
@samtstern
Copy link
Contributor

Sorry I forgot about this, I am fine with this change!

@samtstern samtstern merged commit 226808b into firebase:version-2.1.0-dev Jul 11, 2017
@SUPERCILEX SUPERCILEX deleted the auth-instances-cleanup branch July 11, 2017 17:48
@SUPERCILEX
Copy link
Collaborator Author

@samtstern awesome possum! 😀

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