Skip to content
This repository has been archived by the owner on Apr 2, 2021. It is now read-only.

Integrate latest FBSDK on iOS and JS #541

Merged

Conversation

redcancode
Copy link
Contributor

@redcancode redcancode commented Jun 11, 2019

There have been some breaking changes in the native FBSDK this year which prevents us from using the latest version. I've decided to help the problem and integrate the latest changes with this pull request.

Currently I've integrated the iOS FBSDK changes. My app builds and runs and I was able to login and obtain an access token. I'm not finished with these integrations.

Here are the list of things I want to do:

  • double check iOS FBSDK changes to make sure I've got everything
  • integrate Android FBSDK similarly
  • document all the changes I've made
  • test all the affected features

After I've done with these items this PR should be ready for a real review and consideration to merge it and support the latest FBSDK on both platforms.

It would be great if you could try this out and confirm that it works for you too. Everything should work on iOS.

I've started my branch from the latest published version which is 0.9.0. I did this on purpose because master is broken currently due to RCTComponentEvent still being unpublished.

Your feedback would be greatly appreciated. 🙂

[Update-2019-June-14] This PR has been repurposed into integrating the latest FBSDK on iOS and JS only. The android part has been done in PR #546

Fixes #533

@simonbengtsson
Copy link

Awesome! Will test it with other iOS app before the end of the day.

@simonbengtsson
Copy link

I ran into build issues when trying. At first it was a bolt thing which got resolved after pod deintegrate && pod install. Then it was an issue about 'React/RCTComponentEvent.h' file not found. Got the same when running the official master branch though (#493). You didn't run into this?

@redcancode
Copy link
Contributor Author

redcancode commented Jun 12, 2019 via email

@simonbengtsson
Copy link

You are right! My mistake was specifying github:redcancode/react-native-fbsdk instead of github:redcancode/react-native-fbsdk#integrate-latest-fbsdk. Everything seems to be working in our project.

@redcancode
Copy link
Contributor Author

redcancode commented Jun 13, 2019 via email

@simonbengtsson
Copy link

simonbengtsson commented Jun 13, 2019

I was a bit quick in posting that comment, wrote on updated comment above. The issues mentioned went away after first fixing compile issues from upgrading to v5 of the native sdk in our native app before upgrading react-native-fbsdk.

@redcancode
Copy link
Contributor Author

Those are valid errors. If you pull in the Share subspec of react-native-fbsdk you will get those build errors because there were changes in FBSDKShareKit too. I'm already working on fixing them.

@janicduplessis
Copy link
Contributor

janicduplessis commented Jun 13, 2019

@redcancode Thanks a lot for working on this! I just updated the example app in the repo to target RN 0.60 which fixes the RCTComponentEvent error. My goal here is to publish a new version that targets RN 0.60+ and fbsdk@5+.

Sadly it won't be backwards compatible but users are free to stay on an older version until RN can be updated.

If you are okay with that I can merge this PR as is and work on fixing the remaining errors in FBSDKShareKit

@redcancode
Copy link
Contributor Author

@janicduplessis I'm not ready with this PR yet. I've made progress with the integration of ShareKit on iOS, might be pretty close to being done with it.

I think it would be a great idea to create a new branch for this effort called something like integrate-fbsdk-v5 we could build that branch together and only merge to master when everything is ready on iOS and Android.

What do you say? 🙂

@janicduplessis
Copy link
Contributor

@redcancode master has been in pretty bad shape for a while so we can just work there directly to get it back in good shape ASAP.

I think a good way to split this work is you can make the required changes for iOS / JS and I'll take care of android.

@janicduplessis
Copy link
Contributor

Also if you want to make testing easier you can try the new example app, the instructions to run it are here

@redcancode
Copy link
Contributor Author

@janicduplessis Okay let's use master then and fix it up. In this case I will change this PR so it will be about integrating CoreKit and LoginKit on iOS and JS. That way you can merge it and we can iterate faster.

I will create another PR about ShareKit later which could include other improvements you notice on the way, like the one you commented above.

- Remove deprecated login behaviors
- Rename FBSDKLoginManagerRequestTokenHandler
- Simplify loginWithPermissions call
- Implmenet access token changes
- Rename FBSDKGraphRequestHandler
- Remove deprecated AppInviteDialog
- Remove previewPhoto from VideoContent
- Correct FBSDKShareOpenGraphAction instance creation
- Remove deprecated LikeView
@redcancode redcancode changed the title Integrate latest FBSDK Integrate latest FBSDK on iOS and JS Jun 14, 2019
@redcancode redcancode marked this pull request as ready for review June 14, 2019 07:03
@redcancode
Copy link
Contributor Author

@janicduplessis i've finished with all the changes. also updated the original PR description. the only problem is that i can't build the example app because of some pods path issue. i will solve that later this week. i was able to build my app with these changes though

Copy link
Contributor

@janicduplessis janicduplessis left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for working on this! Left a few minor comments and this is good to go.

Let me know if you figure out the issue with the example app

@janicduplessis
Copy link
Contributor

Actually lets just merge this and you can address the feedback in a followup. I'll work on updating the login method to have a single one instead of loginWithReadPermissions and loginWithPublishPermissions.

@janicduplessis janicduplessis merged commit 628b55a into facebookarchive:master Jun 14, 2019
@redcancode redcancode deleted the integrate-latest-fbsdk branch June 15, 2019 11:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use of undeclared identifier 'FBSDKLoginBehaviorNative'; did you mean 'FBSDKLoginBehaviorBrowser'?
4 participants