search

rnc-archive / apple-authentication

Sign In With Apple for React Native
113 stars 13 forks source link

API changes based on feedback #4

Closed matt-oakes closed 5 years ago

matt-oakes commented 5 years ago

Thanks for the feedback from @Jobeso and @brentvatne on the API design (#1). I have made a couple of tweaks. Hopefully, this should make the use of enums clearer and name the methods in a way that we can include this library in Expo.

Let me know if you have anymore feedback.

Jobeso commented 5 years ago

I really like it, nice improvements.

Did you think about the renaming suggestion to AppleSignIn again? What are your thoughts?

ide commented 5 years ago

It might be useful to introduce methods like loginAsync(...) rather than requestAsync(LOGIN, ...) since the arguments and return values may be quite different for logging in and logging out. (I like what you've done with addRevokeListener() instead of addListener('revoke') for similar reasons.)

But I think that is easy to layer on afterwards and nothing blocking v1.

Another question: was the web version of this component (I believe Apple has a JS library) accounted for in the API design? https://developer.apple.com/documentation/signinwithapplejs

brentvatne commented 5 years ago

looks good to me, I agree with the above comments but they're not blockers

matt-oakes commented 5 years ago

Did you think about the renaming suggestion to AppleSignIn again? What are your thoughts?

I was actually thinking if going for AppleAuthentication. The reason being there are actually a couple of other sign in methods which are not accounted for in the current spec for this library, like Enterprise SSO. The button component would have the same name though.

Apple refers to it as "Sign In with Apple", which I agree is awkward, but I think it's best to use the official name.

It might be useful to introduce methods like loginAsync(...) rather than requestAsync(LOGIN, ...) since the arguments and return values may be quite different for logging in and logging out. (I like what you've done with addRevokeListener() instead of addListener('revoke') for similar reasons.)

I like that! I'll make the change and see how it looks.

Another question: was the web version of this component

Not yet. I'll look into this.

radex commented 5 years ago

AppleAuthentication

I'm leaning in favor of this actually @matt-oakes, as AuthenticationServices also supports other useful methods — check out:

https://developer.apple.com/videos/play/wwdc2019/516/

Specifically this slide:

image

You can, in one request, ask system if it knows if the user is already signed up using either Sign In With Apple or password-based method (iCloud Keychain via Safari)

matt-oakes commented 5 years ago

I'm going to merge this is and rename the library to apple-authentication. Until we start implementing it, I won't update any of the export names as it might make more sense what they should be once we have a working implementation :+1: