Move Auth notification constant into Core. #155
Conversation
There was a problem hiding this comment.
This PR looks awesome. There are a couple additional things that I'd like to see (to make it possible for SDK components to consume these FIRApp APIs without the hackery database does), but we can tackle that separately if you don't want to add to this PR. This is already a great improvement.
| * are invoked by name we can write code against this protocol as long as the method signatures don't change. | ||
| * | ||
| * TODO: Consider weak-linking the actual Firebase/Core framework or something. | ||
| */ |
There was a problem hiding this comment.
Thank you for cleaning all this up!!!
|
|
||
| @end | ||
|
|
||
|
|
||
| /** | ||
| * This is a hack that defines all the methods we need from FIRAuth. | ||
| */ | ||
| @protocol FIRFirebaseAuthLike <NSObject> |
There was a problem hiding this comment.
Erg... I forgot about this. It's another hack that will need to be copied anywhere else somebody tries to consume this notification. Maybe we should add a FIRAuthStateDidChangeInternalNotificationAppKey that contains the FIRApp instance in the notification object?
There was a problem hiding this comment.
Good idea! I'll add it.
|
|
||
| @end | ||
|
|
||
| /** | ||
| * This is a hack that copies the definitions of Firebase Auth error codes. If the error codes change in the original code, this | ||
| * will break at runtime due to undefined behavior! | ||
| */ | ||
| typedef NS_ENUM(NSUInteger, FIRErrorCode) { | ||
| /*! @var FIRErrorCodeNoAuth | ||
| typedef NS_ENUM(NSUInteger, FIRMockErrorCode) { |
There was a problem hiding this comment.
Hrm. This is another FIRApp hack that we should ideally remove. The consumer can't tell if [FIRApp getTokenForcingRefresh:] actually tried to get a token and failed (e.g. no network connection so we can't talk to the backend) or if the app just doesn't use auth (so we should proceed unauthenticated), or because the app uses auth but no user is currently signed in (so again we should proceed unauthenticated).
Any chance we can tackle this too (move these errors into core perhaps) here or in a subsequent PR?
There was a problem hiding this comment.
I'll take a look - I wasn't actually sure what this mapped to. There was already FIRErrorCode declared but doesn't seem to match up with the values here. Do you know where these were copied from?
There was a problem hiding this comment.
Oh, interesting! I'm pretty sure these were errors copied from FIRAuth, but I can't find them either... and looking at the implementation of [FIRApp getTokenForcingRefresh] and the getTokenImplementation that auth provides, I believe our callback will just be called with a nil token if auth is not configured or there is no user signed in... which is exactly what database wants anyway.
So my best guess is that this is some legacy error handling that's no longer needed. We should ideally rip it out, but if we're nervous, we could just add a big comment that this is probably legacy and should be removed.
There was a problem hiding this comment.
Yes, this is legacy code. FIRAuth used to callback with error codes FIRAppErrorCodeNoAuth and FIRAppErrorCodeNoSignedInUser, but as part of the transition from FIRFirebaseApp to FIRApp before we launched publicly last year, both error codes are removed in these cases, and a nil error is returned instead along with a nil user. See b/27696080 for code history. I see no harm removing the legacy code, but it's really up to you two.
There was a problem hiding this comment.
Okay, looking at the other implementations, it looks alright to just forward the token and error to the local callback via callback(token, error);, or would you prefer I wrap it in case it changes in the future? Ex:
if (error != nil) {
callback(nil, error);
} else {
callback(token, nil);
}
There was a problem hiding this comment.
Awesome, thanks @XiangtianDai for digging up the history and confirming. @ryanwilson I think it's fine to just forward directly.
|
Does this PR address #160? |
|
@paulb777 I'll look into it and see if I can address it as well. |
|
FYI- This PR looks good to me. Thanks for the additional changes, Ryan. I think #160 is probably mostly unrelated (I guess database needs to listen for the delete notification and clean itself up?). |
|
Had to solve a merge conflict - will be testing this tomorrow and if all goes well, submitting it tomorrow as well. |
|
This PR's diff looks corrupted. It should not show all the diffs from the merges - it should only show the diff of the PR to master. Probably need to rebase to latest master, verify the diff, and force push. Let's discuss. |
SDKs that want to listen for the internal Auth notifications needed to copy the notification strings to their own SDK instead of relying on Auth's definition in order to avoid a dependency on Auth. By moving them to Core, SDKs can use the constants without taking on another dependency.
ryanwilson
force-pushed
the
rw-auth-listener-core
branch
from
fad3e03 to
0d70209
Compare
|
Thanks Paul, fixed the issue. Re: #160 I'll address that in a different PR. |
|
Tested with test project and build CocoaPod, should be good to go! Will land it. |
|
Thanks again for getting this all cleaned up! 👍 |
* Move Auth notification constant into Core. SDKs that want to listen for the internal Auth notifications needed to copy the notification strings to their own SDK instead of relying on Auth's definition in order to avoid a dependency on Auth. By moving them to Core, SDKs can use the constants without taking on another dependency. * Remove Auth stub, add app instance to notification.
SDKs that want to listen for the internal Auth notifications needed
to copy the notification strings to their own SDK instead of relying
on Auth's definition in order to avoid a dependency on Auth. By moving
them to Core, SDKs can use the constants without taking on another
dependency.