Skip to content
This repository has been archived by the owner on Oct 24, 2022. It is now read-only.

Update Facebook SDK 4.31.1 for iOS 11 #634

Merged
merged 3 commits into from
Mar 23, 2018
Merged

Update Facebook SDK 4.31.1 for iOS 11 #634

merged 3 commits into from
Mar 23, 2018

Conversation

peterpeterparker
Copy link
Collaborator

@peterpeterparker peterpeterparker commented Mar 18, 2018

The following PR follows the reported issue #631

Context

Since a couple of days, Facebook displays a warning in their developers console in order to ask them to upgrade their Facebook iOS SDK

What does that PR contains

  1. This PR contains an update of the Facebook SDK 4.31.1 which could be found at the following address https://developers.facebook.com/docs/ios/componentsdks/

  2. I have updated the showDialog method FacebookConnectPlugin.m, respectively removed the code regarding contentTitle/imageURL/contentDescription because these are now marked as readOnly by Facebook / where deprecated in Facebook API v.2.9

  3. I have updated the README to add a notice regarding these deprecation in the chapter share

What did I test

Using my app (productive app published in stores) I was able to succesfully

a. Build the plugin

b. Test both the login process and share process with a test version of my app including this PR on my test phone (iPhone 6s / iOS 11.2.6)

c. Test the login process with a test version of my app including this PR in a simulated iPhone (iPhone 6 / iOS 9.1)

Note regarding Login process and new popup on iOS 11

It look like that the this new Facebook SDK introduce a new additional popup to the login process with Facebook with iOS 11

Before this SDK:

Button click in my app -> Facebook www opened -> click -> popup to go to FB -> FB -> Accept terms -> Back in app

New with this SDK:

Button click in my app -> NEW POPUP -> Facebook www opened -> click -> popup to go to FB -> FB -> Accept terms -> Back in app

This new popup ask the user if he/she's agree to go from the app to Facebook www

Side note

I went manually thru all differences of all declarations files (.h files) and except the above deprecated variables I didn't noticed any major changes respectively no methods or variables or parameters where removed

Final note

I'm definitely no Cordova plugin or neither Objective-C expert, tried my best. Plz be careful while merging the PR.

Final thought

For the future, maybe instead of being included in the plugin itself, shouldn't be the Facebook library be included thru CocoaPods? Just an idea...

@ThorvaldAagaard
Copy link

I can test this. But unsute how to update my app with your PR. Can you provide me with a link to your repo?

@peterpeterparker
Copy link
Collaborator Author

peterpeterparker commented Mar 18, 2018

@ThorvaldAagaar sure

cordova plugin add https://github.com/peterpeterparker/cordova-plugin-facebook4.git#ios11  --save --variable APP_ID="123456789" --variable APP_NAME="myApplication"

@booleanbetrayal
Copy link

Thanks @peterpeterparker -- Plan on testing this out tomorrow. Maybe you should just ask @jeduan for maintainer status to get this in and put up a README note for Maintainers Wanted?

@peterpeterparker
Copy link
Collaborator Author

cool @booleanbetrayal

Regarding the fact, like I said, that I'm not a pro in this type of PR, I would definitely feel more comfortable about this if a maximum of users test it ;)

About the maintainers wanted, sure I guess it would be cool if jeduan add a note in the README about that

@jeduan
Copy link
Owner

jeduan commented Mar 19, 2018

@peterpeterparker added you as collaborator on this project. Let me know your npm username so you can publish new versions.

@peterpeterparker
Copy link
Collaborator Author

peterpeterparker commented Mar 19, 2018

@jeduan I feel honored, not sure I have enough know-how for this so I will be really careful

my npm username is peterpeterparker too

@peterpeterparker
Copy link
Collaborator Author

I have published a post on the Ionic forum to try to find some more testers.

https://forum.ionicframework.com/t/need-testers-for-cordova-plugin-facebook4-pr

The more we test this on different phones and different apps, the better it is

@booleanbetrayal
Copy link

booleanbetrayal commented Mar 20, 2018

I'm seeing auth work as expected (with the new confirm modal) on an iPhone-6s iOS 11 physical device. Seems like the confirm is presented each time, which is a bit obnoxious.

@peterpeterparker
Copy link
Collaborator Author

@booleanbetrayal thx for testing this PR 👍

What do you mean with "Seems like the confirm is presented each time", except the "new confirm modal" did you noticed something different? could you display that with a screenshot?

If you are speaking about the "new confirm modal", like you, I'm not a big fan, I think that makes too much modals

@booleanbetrayal
Copy link

Yeah, I just meant that I'm not a big fan of that new modal in the flow @peterpeterparker . My experience sounds consistent with the intended behavior. So everything LGTM from what I can tell with this PR.

@peterpeterparker
Copy link
Collaborator Author

@booleanbetrayal thx for the feedback, all clear then. I think it's good to collect a couple of more tests before merging this, I really don't want to screw something

about the new modal flow I'm with you, I don't like it neither. I don't know what we could do, maybe nothing. Maybe an alternative would be now to implement a 100% web Facebook login based? don't know if that could be a good solution...should analyze this

@jeduan
Copy link
Owner

jeduan commented Mar 21, 2018

@peterpeterparker Added you to npm. I think you shouldn't do anything related ugly modals. Fighting people who want a different modal from Facebook is hugely draining and there are better ways to spend your time.

Re: testing, people generally don't upgrade that often (requires rebuilding the app) so if you have a couple of tests, you should go ahead and publish it.

@jeduan
Copy link
Owner

jeduan commented Mar 21, 2018

Regarding cocoapods, I think you should do it. I tried a few ways, including moving the SDK to a separate npm module, Carthage and git submodules with a post-install script but nothing worked when this plugin was created. If cocoapods is supported now, go for it.

@fredgalvao
Copy link
Collaborator

tl;dr: Cocoapods dependencies on cordova work well.

@jeduan @peterpeterparker The push plugin successfully uses cocoapods dependencies to pull the FCM code on the iOS side of things. It works, even thought the installation process and the project management on xCode is different.

@peterpeterparker
Copy link
Collaborator Author

@jeduan @fredgalvao

I'm a bit under pressure but as soon as I find time I'll publish this PR (I'll bump up version of the plugin to 1.10.0)

About cocoapods, I was thinking about the example of push plugin too. I'll open a feature issue so we will still think about it

@peterpeterparker
Copy link
Collaborator Author

This has been now release to npm

Plugin version v1.10.0

@booleanbetrayal @jeduan iOS 11.3 improve the situation with the modal, there isn't anymore "two modals" and the login flow looks even clean

@peterpeterparker
Copy link
Collaborator Author

For the record, an update of my app including the last update of the plugin aka including this Facebook SDK for iOS has been published in store respectively successfully reviewed by Apple

Therefore, all good

Lindsay-Needs-Sleep pushed a commit to miloproductionsinc/cordova-plugin-facebook-connect that referenced this pull request Mar 7, 2020
* Update FacebookSDKs-iOS-4.31.1

* contentTitle, imageURL and contentDescription have been deprecated in Facebook API 2.9

* Update README according deprecation
Lindsay-Needs-Sleep pushed a commit to miloproductionsinc/cordova-plugin-facebook-connect that referenced this pull request Mar 7, 2020
* Update FacebookSDKs-iOS-4.31.1

* contentTitle, imageURL and contentDescription have been deprecated in Facebook API 2.9

* Update README according deprecation
Lindsay-Needs-Sleep pushed a commit to miloproductionsinc/cordova-plugin-facebook-connect that referenced this pull request Mar 7, 2020
* Update FacebookSDKs-iOS-4.31.1

* contentTitle, imageURL and contentDescription have been deprecated in Facebook API 2.9

* Update README according deprecation
Lindsay-Needs-Sleep pushed a commit to miloproductionsinc/cordova-plugin-facebook-connect that referenced this pull request Mar 7, 2020
* Update FacebookSDKs-iOS-4.31.1

* contentTitle, imageURL and contentDescription have been deprecated in Facebook API 2.9

* Update README according deprecation
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants