Skip to content
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(sdks): Update to v13 of native SDKs #221

Merged
merged 9 commits into from Apr 18, 2022

Conversation

isidoro98
Copy link
Contributor

@isidoro98 isidoro98 commented Apr 4, 2022

This is a PR for updating the library to the latest FB sdk - Fixes #208

Facebook post introducing the new version here

iOS Changes

Please refer to the official changelog here, main things to note are:

The minimum supported version of iOS is now 11.0 for all frameworks. 
The minimum supported version of tvOS is now 11.0 for all frameworks. 
The XCFramework binaries are now built with Xcode 13 so Xcode 12 is no longer supported.
Starting with the v13.0 release, client tokens must be included in apps for Graph API calls. 
An exception is now raised when running apps without a client token.

Breaking changes

None

Android Changes

Please refer to the official changelog here, main things to note are:

Set Java source and target compatibility to 1.8 (Java 8). 
All apps that integrate with Facebook Android SDK should also set source and compatibility to 1.8 or above.
GMS AD_ID Permission is added to the SDK by default for requesting the advertising ID
Removed the support for tokenless requests. 
Now all Graph API requests must be associated with an access token and the app needs to have a client token to construct the access token without a logged in user.

Breaking changes

On android the deprecated ShareLink parameters contentDescription, contentTitle and imageUrl have now been removed.

Regarding the testing plan I would appreciate suggestions as well.

@isidoro98 isidoro98 changed the title [WIP]: Update to v13 of native SDKs feat(ios, sdk): [WIP]: Update to v13 of native SDKs Apr 4, 2022
@isidoro98 isidoro98 changed the title feat(ios, sdk): [WIP]: Update to v13 of native SDKs feat(ios/android, sdk): [WIP]: Update to v13 of native SDKs Apr 4, 2022
@isidoro98 isidoro98 changed the title feat(ios/android, sdk): [WIP]: Update to v13 of native SDKs feat(ios/android, sdk): [WIP] Update to v13 of native SDKs Apr 4, 2022
@isidoro98 isidoro98 changed the title feat(ios/android, sdk): [WIP] Update to v13 of native SDKs feat(ios/android, sdk): Update to v13 of native SDKs Apr 5, 2022
@mikehardy mikehardy changed the title feat(ios/android, sdk): Update to v13 of native SDKs feat(sdks): Update to v13 of native SDKs Apr 7, 2022
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Thank you so much for posting this

Have you test-integrated it in to an app? You can do so by creating a patch-package patch from inside the example with your changes, then using that in a work project - if the example works and your use cases work then I think technically it's good.

With breaking changes the biggest thing is documentation either directly in the changelog (my favorite) or in a migration guide linked from the changelog, and updating the docs

I left a comment related to the share dialog, it looks like the docs need an update?
After that I think the breaking changes to call out are:

  • ios min version moved to 11
  • you must use Xcode 13
  • share dialog parameters changed as mentioned
  • you must use configure and use a client token to use Graph API calls
  • GMS AD_ID Permission is added to the SDK by default, use Android Manifest merge tools to remove it if unwanted

Anything else? That was what I got from your PR description. That's short enough I can include it as a breaking changes entry on the merge so it shows up in the changelog

@mikehardy mikehardy added the needs more info waiting on original poster to provide details label Apr 7, 2022
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay - I think this looks good.

For anyone reading this now we need testers! That is what will gate this from release. I would like to hear one other person say they have test-integrated it and it works for them, ideally that uses some of the other modules (login and events seem to be covered).

I can do this but I am very short on time so it might be a few days or more. Worst case, if it sits for a week I'll just release it and if there are issues, there are issues and we do x.0.1 release

Either way, fantastic work @isidoro98 thank you!

@mikehardy
Copy link
Collaborator

maintainer note: squash merge with this in commit message:


BREAKING CHANGE: this version uses new major versions of the underlying SDKs and has breaking changes:

  • iOS mininum target version moved to 11, alter this in your Podfile and in project.pbxproj as needed
  • You must use Xcode 13 going forward. Xcode 12 is no longer supported
  • Share dialog parameters changed, description and title were deprecated, they are removed now
  • You must use configure a client token on Facebook console and in the app to use Graph API calls now
  • GMS AD_ID Permission now added by the native SDK by default, use AndroidManifest tools:remove to remove if unwanted

@mikehardy mikehardy added pending-merge Just waiting on clean CI test run. Any maintainer should feel free to merge if CI is passing. and removed needs more info waiting on original poster to provide details labels Apr 8, 2022
@mikehardy
Copy link
Collaborator

I posted the patch-package that contains this change on the related issue #208 (comment)

@abumalick
Copy link

Thanks for working on this @isidoro98 . I tested the build on expo managed app via the plugin from the repository. The expo-dev-client build passed, on android it seems to work. On iOS, it crashes when I launch the app with this error:

*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Starting with v13 of the SDK, a client token must be embedded in your client code before making Graph API calls.
Visit https://developers.facebook.com/apps/738908110438103/settings/advanced/ to find your client token for this app.
Add a key named FacebookClientToken to your Info.plist, and add your client token as its value.
Visit https://developers.facebook.com/docs/ios/getting-started#configure-your-project for more information.'

The facebook client token is not mandatory for iOS. We should add this to the documentation right? This was not required before and is also a breaking change for iOS.

In the expo plugin (plugin folder at the root of the repo) we could make the build throw an error if clientToken is not passed also (But I understand if you prefer to do this on another pull request)

I am adding the client token and give more feedback inchallah.

@mikehardy
Copy link
Collaborator

Needing a client access token is definitely a breaking change, this will be a breaking change release as noted in my proposed changelog notes. If the Expo plugin needs an update for this we'll need someone with experience to handle it, you can do that right now in parallel on a separate PR, no need to wait. Optionally see if there is an fbsdk client access token for ios in app.json and do the needful if it exists? it will work on v12 already

@abumalick
Copy link

I don't see that the client token is necessary in the readme for native instructions. Shouldn't we add something there?

@mikehardy
Copy link
Collaborator

I don't see that the client token is necessary in the readme for native instructions. Shouldn't we add something there?

Yes! Good catch. It was optional before but now is definitely mandatory. That's a README update we should add in this PR if you can @isidoro98 or we can do a follow-up

@abumalick
Copy link

If the Expo plugin needs an update for this we'll need someone with experience to handle it, you can do that right now in parallel on a separate PR, no need to wait.

I am a bit busy these days, I will try to work on this this weekend or in the beginning of the week inchallah

@isidoro98
Copy link
Contributor Author

I don't see that the client token is necessary in the readme for native instructions. Shouldn't we add something there?

Yes! Good catch. It was optional before but now is definitely mandatory. That's a README update we should add in this PR if you can @isidoro98 or we can do a follow-up

The readme is already linking to facebook's getting started Guide of both iOS and android, where it explains the configuration step by step, including adding the client token. The README itself doesn't get into detail regarding how to configure the app.

Can be a troubleshoot step, what do you guys think?

@mikehardy
Copy link
Collaborator

The readme is already linking to facebook's getting started Guide of both iOS and android, where it explains the configuration step by step, including adding the client token. The README itself doesn't get into detail regarding how to configure the app.

Can be a troubleshoot step, what do you guys think?

Ahh, good point. Maybe a "I just upgraded to v13 and it says I need an access token"
Since we defer to the upstream getting started guide, I think that (for new users) plus the changelog (for upgrades) plus a troubleshoot step would handle 110% of all cases ;-)

@mikehardy
Copy link
Collaborator

I'm going to merge this - the Expo plugin update I will peel off to a new issue, for people that need the Expo plugin to work they'll have to stick on v12 until it's updated

@mikehardy mikehardy merged commit 493a638 into thebergamo:master Apr 18, 2022
@mikehardy mikehardy removed the pending-merge Just waiting on clean CI test run. Any maintainer should feel free to merge if CI is passing. label Apr 18, 2022
github-actions bot pushed a commit that referenced this pull request Apr 18, 2022
## [8.0.0](v7.3.3...v8.0.0) (2022-04-18)

### ⚠ BREAKING CHANGES

* **sdks:** this version uses new major versions of the underlying SDKs and has breaking changes:

- iOS mininum target now 11, alter your Podfile and in project.pbxproj as needed
- Xcode 13+ required going forward. Xcode 12 not supported
- Share dialog description and title were deprecated in v12,  removed now
- Client access token required: configure on Facebook console and in the app
- GMS AD_ID Android Permission added by native SDK by default, use AndroidManifest tools:remove if unwanted

Please note the Expo config plugin has not been updated yet to handle client access token, use v12 if you need Expo config plugins. Follow #228 for updates or post a PR if this is important to you. Thanks!

### Features

* **sdks:** Update to v13 of native SDKs ([#221](#221)) ([493a638](493a638))

### Bug Fixes

* **jest:** add LoginManager.logOut + AccessToken mocks ([#223](#223)) ([5ba05f3](5ba05f3))
@github-actions
Copy link

🎉 This PR is included in version 8.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to v13 of native SDKs [testers wanted! patch-package patch ready!]
3 participants