feat: create expo config plugin #205
Conversation
There was a problem hiding this comment.
This looks great - I'll approve the CI run and hopefully it goes green.
I had a comment about the docs for the config elements and I'm curious for your thoughts there
It is really important to note that I do not use or test Expo functionality, so I will not be able to support this at all. I'm more than happy to have it in the library as it will open the library to a large pool of possibly interested users but I just won't be able to help any of those users with troubleshooting or maintain this code, so ongoing help would be more than appreciated. Is that something you can provide?
| */ | ||
| scheme?: string; | ||
|
|
||
| clientToken?: string; |
There was a problem hiding this comment.
I think it is important to at least mention that you pull this from the facebook developer dashboard as well, and that it will be required in the next major version of the facebook SDKs so you should configure it even if it is temporarily optional.
Thinking further: many of these ConfigProps are marked as optional, but, the appID and displayName and scheme etc - some of those are required and not optional, aren't they?
There was a problem hiding this comment.
I make some config are optional because I want to support Expo User moving from expo-facebook to this package. They ready has some config in the app.json so they don't have to declare it twice. But, I will add validation for missing appID and displayName. (scheme will default to fb + appID).
facebookAppId,
facebookDisplayName,
facebookScheme,
facebookAutoInitEnabled,
facebookAutoLogAppEventsEnabled,
facebookAdvertiserIDCollectionEnabled,
mikehardy
added
the
needs more info
Yes, I can help to maintain this code/troubleshoot any issue with Expo. |
There was a problem hiding this comment.
Fantastic, and I can't tell you how much I appreciate the offer to help on maintaining it, I have limits to what I'm familiar with + have time to become familiar with and while I love the idea of Expo users being able to take advantage of the module I'm just at my limit now and don't personally don't know Expo 😅
This looks good to me now - I like the validation, let's see what CI says :-)
mikehardy
added
pending-merge
|
I think the CI was green: https://github.com/thebergamo/react-native-fbsdk-next/actions/runs/1887573023 |
mikehardy
removed
the
pending-merge
## [7.3.0](v7.2.0...v7.3.0) (2022-02-23) ### Features * create expo config plugin ([#205](#205)) ([d44e360](d44e360))
|
🎉 This PR is included in version 7.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #192
Test Plan: