v1 API Discussion

[matt-oakes]

I have gone ahead and created a detailed README for a possible API that we could implement for this library. You can see the details of it here:

https://github.com/react-native-community/apple-sign-in/blob/46aab22632ca482e737f6447a480057648c07710/README.md

All of this is only a suggestion and we should discuss if this API makes sense and open PRs to make changes to it as required.

Reasoning for the proposed API

The goal is to come up with an API that is close to the one provided by Apple, but makes sense in a React context. I have tried to stick to the names that Apple has used and provide access to all of the options which they allow developers to call. We want to make sure our API is as flexible as the iOS one we are wrapping.

Open questions

• Are we happy with the ways the different parts of the library are exported?
  • There are currently two main exports (SignInWithApple and SignInWithAppleButton) and the types.
• Does the way we are exporting enums make sense?
  • Are we happy with them being under the relevant main export (SignInWithApple or SignInWithAppleButton) or should they be their own main export (SignInWithAppleButtonType rather than SignInWithAppleButton.Type).

[vonovak]

really good job with the readme! Looks good to me.

There are currently two main exports

👍

Are we happy with them being under the relevant main export

👍

re https://github.com/react-native-community/apple-sign-in#signinwithappleaddrevokelistener - @matt-oakes can you point me to the relevant apple docs?

regarding styling the button docs there are two initializers that influence the button styling, but we instantiate the button here
https://github.com/react-native-community/apple-sign-in/blob/f33deb1fd584e94f4019b3aae9a474618bcd3e1b/ios/RNCAppleSignInButtonManager.m#L14

and I don't know if the js props are accessible in there.. Anyone has experience with that? Other native view managers seem to simply instantiate views and then use setters to set various properties example

[matt-oakes]

"Documentation" is strong word for what Apple provide for the revise notification:

https://developer.apple.com/documentation/authenticationservices/asauthorizationappleidprovidercredentialrevokednotification?language=objc

The Swift example code they gave in the presentation was this though:

// Register for revocation notification
let center = NotificationCenter.default
let name = NSNotification.Name.ASAuthorizationAppleIDProviderCredentialRevoked
let observer = center.addObserver(forName: name, object: nil, queue: nil) { (Notification) in
  // Sign the user out, optionally guide them to sign in again
}

[Jobeso]

Awesome that you guys are already planning this. The work you already put in is great! The current approach is also quite nice. Here are a few thoughts of mine to it:

Are we happy with the ways the different parts of the library are exported?

We could name it AppleSignIn like the library name. It has the same meaning as SignInWithApple and is even shorter. Button could then be AppleSignInButton

Does the way we are exporting enums make sense?

I think it makes sense to have them as a part of the main export (like you did it) and not separated from it.

Suggestions

Enums

I propose to name types camel case and as plural cause they container multiple enums. I also think it makes sense to have the actual enums in uppercase and separated with underscores e.g.:

SignInWithAppleButton.types.DEFAULT
SignInWithAppleButton.types.SIGN_UP
SignInWithAppleButton.types.CONTINUE

Thats how I see enums treated commonly.

SignInWithApple.perform()

I think it makes sense to name the function SignInWithApple.request(). This seems for me to be more explicit about what is going to happen and also mirrors other libraries that i.e. deal with permissions. Then we can also rename requestedScopes to scopes because it's already part of the request action.
Moreover I think it makes sense to rename requestedOperation to action because that is more familiar, it's shorter and also mirrors what's going to happen. From my proposal the function would look like this then:

SignInWithApple.request({
  action: SignInWithApple.Action.Login,
  scopes: [],
  user: '',
  state:''
})

I get that you want to stay as close as possible to Apples API but I think it makes sense to do some thinks differently cause react-native is a different eco system and most of the people are more familiar with that than with native. So I think it makes sense to stick to these known patterns. Also calls generally happen differently in native iOS than in JS e.g. the perform() function.

Would be happy to get some thoughts on these ideas.

[matt-oakes]

Thanks for the feedback @Jobeso!

I partly agree with you on the enums case. I have changed all of the values to be SNAKE_CASE, however, I think it reads better when the names of the enums are singular. Some feel very awkward if they are plural.

I also partly agree with you on the method and value names. I have changed perform to request as that is much clearer. I think we should keep the rest the same Apple provides. The reasons for this is that I don't actually see that there is a "standard" for any of this in JS world. Seeing as this library would just be a wrapper for the Apple API, I think we should stick to the language they use as much as possible. This makes it much easier for developers to see details in the Apple Documentation (which they absolutely should be using more!).

I will open a PR now with these changes.

[HommeSauvage]

How about keeping both names? perform and request as an alias, same for scopes and requestedScopes

[matt-oakes]

@redgenie I don't think that gains very much and just increases the API surface area. We'll take a look when we start implementation.

[fbartho]

Is there a plan for an Android Web-based implementation?