rnc-archive / apple-authentication

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

v1 Implementation Discussion #2

Open matt-oakes opened 5 years ago

matt-oakes commented 5 years ago

We should use this issue to track how we are planning to implement v1 of the library.

Open questions

ericlewis commented 5 years ago

Typescript

+1

Swift

-1. Well have to maintain Swift <-> ObjC bindings. We could probably use Sourcery for this, but that’s even more added complexity. Depending on version targeted, the swift library may need to be embedded potentially increasing binary size. There is also the issue of Swift differing between versions. Tho we do have ABI stability now.

vonovak commented 5 years ago

TypeScript

+1

Swift

I'm also in favor of ObjectiveC for one reason - I know it works with unimodules. That way, this module will also work in Expo (see unimodules in expo's google sign in) and we'll avoid the situation when two libraries doing the same thing are maintained - which is now the case with the google sign in repo. (I talked about this with Brent Vatne but didn't have enough time to move forward on the matter.) If we do this from the beginning, we can cover both vanilla RN, expo and potentially flutter I still need to read up more on this to understand what this offers. However, I don't know if using unimodules causes some other problems and I don't know if Expo people would be interested in this - I'll ask directly as I cannot seem to use @ to mention anyone relevant here.

Can we split it into sections that can be worked on in parallel?

I guess so; Maybe discord is better for this. keeping this open would be nice

Jobeso commented 5 years ago

Are we using Typescript?

I think it's easier for people to contribute when it's just JS and prop types for components, if there are huge upsides here or just to have a better support for people using TS it makes sense to use it.

Do we use Swift for the native side?

I like it if libraries use swift and it's definitely possible. It's more beginners friendly and easier to understand than objective-c especially if you come from the JS side. I think it opens up for more contributions. Also handling the swift - objc bindings is not a big deal from my point of view. However, I see the downside in increasing binary size by embedding the necessary swift libraries.

How to we coordinate implementing?

Maybe separate issues for the parts that don't conflict each other. Then certain people can take one over and create a PR.

brentvatne commented 5 years ago

I'm also in favor of ObjectiveC for one reason - I know it works with unimodules. That way, this module will also work in Expo (see unimodules in expo's google sign in) and we'll avoid the situation when two libraries doing the same thing are maintained

this end state of having one module that Expo engineers and the rest of the community all collaborate on is definitely desirable! I don't have a strong preference on whether it's implemented as a unimodule or not, we certainly don't want to try to force a different standard on the community if there isn't an appetite for it beyond compatibility with Expo. we can still include packages in the Expo SDK if they aren't unimodules.

there are a few things that we'd love to see in this module for us to collaborate on it and include it in Expo:


the benefits of using a unimodule here would be:

the downsides:

matt-oakes commented 5 years ago

Thanks for the feedback everyone! I'll review this and come up with some changes to the README based on this.

In the meantime, keep the feedback coming!

vonovak commented 5 years ago

hello everyone, sorry for radio silence, I was too busy.

I don't have a strong preference on whether it's implemented as a unimodule or not ... we can still include packages in the Expo SDK if they aren't unimodules.

in that case I'd lean toward not using the unimodules unless we find out we need access to functionality like permissions, filesystem as @brentvatne mentioned. While the possibility to create one module for flutter and cordova is great, I don't think it's realistic that we'd implement support for those tools. We can always add unimodules later if needed.

there are a few things that we'd love to see in this module for us to collaborate on it and include it in Expo ...

sounds good to me!

I think nothing is keeping us from starting to work on this, as the api is nicely described in the readme. I'll slowly start working on this and hope that @matt-oakes who worked on the api description will be available to do some code review here and there and I know @ericlewis was interested in working on this too.

I plan to proceed roughly like this: 1 . rename the objC module to match repo name (done) 2 . convert existing JS to TS 3 . implement basic JS auth to see if the proposed api can cover both native and JS interfaces 4 . go deeper and add support for missing apis in objective C and JS

Anyone else reading this is ofc also welcome to pitch in. Just let us know here what you want to work on and open a draft PR for visibility.

matt-oakes commented 5 years ago

Happy to help with code reviews! Feel free to open a draft PR whenever you have something. That will allow other to collaborate easily.

brentvatne commented 5 years ago

sounds like a good plan!

tunegov commented 5 years ago

Hi, I really want to help you make this lib, can you tell me what can I do?

P.S. this is my first time of contributing, so do not judge strictly

matt-oakes commented 5 years ago

@tunegov Help is always welcome! Keep an eye on this repo (you can watch it using the buttons at the top of the page to get notifications) and feel free to jump in and help when you see the opportunity.

It seems like there hasn't been too much movement on it yet, but once we have some basics in there there will be plenty of opportunity to help out 👍

vonovak commented 5 years ago

@tunegov you can try introducing typescript to the package, or implement some of the apis described in readme if you want :) For adding TS, https://github.com/react-native-community/bob can serve as inspiration. It should also be easier as it does not require digging into native code.

thib92 commented 5 years ago

Hi! 👋🏼 I’d love to contribute to this repo and I have a bit of background with TypeScript and React Native. I’d love to jump in and try to convert the code base to TS.

I have no experience with native iOS nor Objective-C but I’d love to help out on this as well.

vonovak commented 5 years ago

@thib92 sure, please go ahead! :)

fbartho commented 5 years ago

I’m here to contribute as well. We just got done with SCA (Europe Payments Law coming into effect Saturday, now to keep Apple happy!)

vonovak commented 5 years ago

hi everyone, we are going to stop the development of this package and rely on Expo's implementation instead, please see the updated readme. Thanks!