New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ios)!: upgrade facebook-ios-sdk to v12 #132
Conversation
- minimum iOS deployment is now 10.0, per FBSDK requirements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fantastic, thank you thank you
Things I saw:
- iOS/tvOS 9 -> 10 is breaking
- iOS always returns nil from from getAdvertiserId now
- setAdvertiserTrackingEnabled always returns true now (may not be a change but easy to note with an asterisk)
...I think that's it?
The changes to the docs and making the facebook app id dynamic is a nice touch
Thanks. Re comments:
* Yes, iOS 9 to 10 is a breaking change. Sorry I missed that one in the PR notes
* Yes, getAdvertiserID always returns null for iOS. I think I mentioned that in the PR notes under breaking changes
* setAdvertiserTrackingEnabled returns true; it used to always return YES on success, so this mimics the same behavior.
Thanks...Gerry
On Nov 12, 2021, at 7:35 PM, Mike Hardy ***@***.***> wrote:
@mikehardy approved this pull request.
This looks fantastic, thank you thank you
Things I saw:
* iOS/tvOS 9 -> 10 is breaking
* iOS always returns nil from from getAdvertiserId now
* setAdvertiserTrackingEnabled always returns true now (may not be a change but easy to note with an asterisk)
...I think that's it?
The changes to the docs and making the facebook app id dynamic is a nice touch
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#132 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AD63X4D7Y5CF226SBI3UY3DULWXG3ANCNFSM5H5VFOZQ>.
Triage notifications on the go with GitHub Mobile for iOS<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
No need to apologize, this is truly great. The notes were great. I should have prefaced that with a statement that I was more just "active listening" to make sure I got everything. I'll get this packed up and out the door |
Alright @thebergamo - one more big ol' breaking change queued up, I always like a second set of 👀 on them :-), specifically the commit so you can see the message here: d813153 Release script worked perfectly last time for v5 and should work this time too. I considered (before releasing v5) packing them together since this was already in progress, but just in case there are still people on iOS v9 I split it so the breaking changes are in two steps, and with the previous one they get close to current on the native SDKs but maintain iOS v9. Maybe that's useful? Anyway, I never fear major versions and don't mind them coming quickly as long as the breaking changes notes are written clearly Let me know what you think, and if it's good 🚀 🚀 :-) |
Alright, going to go ahead and launch this after a second look myself. If I break it, I'll try to pick up the pieces :-) but hopefully it just goes. Thanks again @NiwotSmitty |
🎉 This PR is included in version 6.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Upgrade FBSDK to v12.1.0. This fixes #55.
BREAKING CHANGE:
getAdvertiserID
will always return null, as the upstream FBSDK no longer provides the advertiserIDsetAdvertiserTrackingEnabled
always returnstrue
now, as it will never fail; no functional change from previous implementationHere is a summary of the changes:
react-native-fbsdk-next.podspec
to specify FBSCK v12.1.0refresh-example.sh
to extract the Facebook App ID into a variableI tested these changes by running the example and substituting my own local Facebook App ID and verified I could log in and log out with the test app.
Note: My editor trimmed some whitespace from the ends of a few lines in the
README.md
file, so the changes look larger than they actually are. All the content changes I made are near the end of the file.