[ios][facebook] add schemes for facebook login & always handle openURL in EXFacebookAppDelegate #7931

cruzach · 2020-04-20T21:15:04Z

Why

(1) Closes #6459
and (2) closes #6112

(1) Currently, Facebook Login in the Expo Client on iOS does not redirect back to the experience after selecting to "Login via the Facebook app." Instead, the user is left with a blank white screen with Cancel at the top left.

(2) In standalone apps (and the Expo Client once (1) is addressed), after calling Facebook.loginWithReadPermissionsAsync & signing in with the facebook app, the webbrowser window occasionally remains open after the native facebook app redirects back to the standalone app after the user has logged in, thus users can never log in. This is because UMAppDelegateWrapper is iterating over the subcontractors looking for a viable openURL method. Sometimes ExpoKitAppDelegate is first in the set, other times EXFacebookAppDelegate is first. Since ExpoKitAppDelegate always returns YES for openURL, if it’s first in that list, it attempts to (and says it has successfully) open the FB url, but it should not

How

(1) By adding the fbid url scheme, the FacebookAppId, and the required LSApplicationQueriesSchemes from Facebook's documentation, the redirect is now successful. The scoped facebook module now also automatically uses the Expo Client's facebook app id, even if the user provides one. This way, developers don't have to remember to change their app id in place of a "development" app id (Expo's) when they build or publish for release, they can write the method once and forget about it.

(2) By adding a priority property to singleton modules, which defaults to 0, we can re-order the subcontractors to match this priority. So UMAppDelegateWrapper would order unimodules:
n , … , 1, [all the 0s in whatever order], -1, -2, ..., -n

Test Plan

Tested in a local client build with app.json facebook configuration, and without (but providing in the Facebook.initializeAsync call)

Confirmed that ExpoKitAppDelegate is always last in the array of subcontractors

sjchmiela

Thanks for taking care of this issue, it giving us and the developers a hard time for so long… 😃

What are the consequences of using Expo's Facebook app for all API calls in Expo Client? Should we list them in the documentation? We should probably also put that information somewhere around a list of breaking changes introduced in SDK37.

Could you also please add some people from the SWM team to the Expo Client Facebook app, so in case something goes wrong we can at least look into the settings and see if everything is configured as it should?

docs/pages/versions/unversioned/sdk/facebook.md

ios/Exponent/Versioned/Core/UniversalModules/EXFacebook/EXScopedFacebook.m

ios/Exponent/Supporting/Info.plist

sjchmiela · 2020-04-22T08:16:39Z

ios/Exponent/Supporting/Info.plist

+		<string>fbapi20150629</string>
+		<string>fbapi20160328</string>
+		<string>fbauth</string>
+		<string>fb-messenger-share-api</string>


Do we still need https://github.com/expo/expo-cli/blob/6bacea668e954c24eb8d9ed6d46a5c46bc3e4a2f/packages/xdl/src/detach/IosNSBundle.ts#L317-L322? Should we .uniq() the array before building the app, add/remove some entries?

hm so it seems the facebook docs are conflicting a little bit:

when setting up the app it at the facebook developer console it says to include all of these (it know's we use FB SDK v5)

but here it says that the fbapi.* schemes are only if you are on FB SDK v4.5 or older

I haven't noticed any issues when excluding the schemes, so probably safe to remove them

ios/Exponent/Supporting/Info.plist

ios/Exponent/Versioned/Core/UniversalModules/EXFacebook/EXScopedFacebook.m

cruzach · 2020-04-23T13:10:11Z

What are the consequences of using Expo's Facebook app for all API calls in Expo Client?

The main drawback is that they won't see app events logged, since their app isn't being initialized. Possibly also ramifications for facebook ads, since that relies on facebook app id as well

Could you also please add some people from the SWM team to the Expo Client Facebook app, so in case something goes wrong we can at least look into the settings and see if everything is configured as it should?

Absolutely, I just need their facebook user name (found by navigating to your facebook profile page, and checking the URL- facebook.com/<user-name> )

sjchmiela

These changes look great, great job! 👍

sjchmiela · 2020-04-23T11:23:52Z

packages/@unimodules/core/ios/UMCore/UMSingletonModule.h

@@ -6,6 +6,8 @@ NS_ASSUME_NONNULL_BEGIN

 @interface UMSingletonModule : NSObject

+@property (nonatomic) int priority;


Could we make it either a readonly property or a method? This way we'd make it obvious that the only way to override value for this property is through subclassing, not manipulating the object. We could provide a default implementation for the method in UMSingletonModule.m.

Could we also make it an NSInteger? I think it's more Objective-Cy. 🤔

packages/expo-facebook/ios/EXFacebook/EXFacebookAppDelegate.m

sjchmiela · 2020-04-23T11:39:31Z

packages/@unimodules/core/ios/UMCore/UMAppDelegateWrapper.m

+
+    NSSortDescriptor *sortDescriptor = [[NSSortDescriptor alloc] initWithKey:@"priority"
+                                                                   ascending:NO];
+    [subcontractors sortUsingDescriptors:[NSArray arrayWithObject:sortDescriptor]];


ios/Exponent/Versioned/Core/UniversalModules/EXFacebook/EXScopedFacebook.m

sjchmiela · 2020-04-23T14:23:46Z

ios/Exponent/Versioned/Core/UniversalModules/EXFacebook/EXScopedFacebook.m

@@ -48,15 +48,16 @@ - (instancetype)initWithExperienceId:(NSString *)experienceId andParams:(NSDicti
    BOOL hasPreviouslySetAutoInitEnabled = [_settings boolForKey:AUTO_INIT_KEY];
    BOOL manifestDefinesAutoInitEnabled = [params[@"manifest"][@"facebookAutoInitEnabled"] boolValue];

-    NSString *facebookAppId = params[@"manifest"][@"facebookAppId"];
+    NSString *scopedFacebookAppId = [[NSBundle mainBundle] objectForInfoDictionaryKey:@"FacebookAppID"];
+    UMLogInfo(@"Overriding Facebook App ID with the Expo Client's. To test your own Facebook App ID, you'll need to build a standalone app. Refer to our documentation for more info- https://docs.expo.io/versions/latest/sdk/facebook/");


Do you think it makes sense to always log that information when the experience is starting? Maybe only if user defines his/her own FacebookAppID (if so, they may expect Expo Client to use it) would suffice?

ios/Exponent/Versioned/Core/UniversalModules/EXFacebook/EXScopedFacebook.m

docs/pages/versions/unversioned/sdk/facebook.md

sjchmiela

🎉 Looks super great to me! 🎉

…L in EXFacebookAppDelegate (#7931)

appsgenie · 2020-04-29T18:21:07Z

does anyone know how we can get this fix into our existing projects? or what the current status is for this fix to get released?

jeanlucnguyen · 2020-05-06T14:58:10Z

Since the Facebook APP ID used now is always the one from Expo when using Expo Client, it seems that we can't have access to user_friends, user_birthday or user_gender permissions anymore. This used to work well when we could use our own APP ID. Also, being forced to use Expo App Id prevents us from using Facebook Test Users feature as well. Isn't it possible to leave an option to be able to use our own Facebook App Id in development when using Expo Client? We never had the problem referenced in this issue before so this seems like a step back for us.

atyrian · 2020-05-09T13:40:02Z

Hi. I am quite new to development, so forgive me if this is not the right forum.

Today I noticed that the permissions that I had previously granted to my application via Facebook's developer portal were no longer effective (user_gender, user_birthday, user_photos). The User ID of my user also changed (I presume this is scoped to the app). After reading the changelog for SDK 37 I now understand that the reason being the user token is issued in the scope of Expo Client instead of the app that I have registered with Facebook.

Is this with the understanding that I may now only use the default public profile permissions, or is there some other way of getting more granular permissions? My application relies very heavily on being able to fetch user images. My understanding is that I should now build my app as a Standalone application (something that I would have to do to distribute the app on the app store anyways?) as per the SDK 37 documentation.

Thank you for any help in this matter.

cruzach · 2020-05-11T14:18:13Z

@atyrian that's correct, you should build your app as a standalone IPA (you can also test using simulator builds) if you want to use your own Facebook app ID

@jeanlucnguyen it seems like those particular permissions, along with a couple other features, require app review, I'll look into that. As for Test Users, it seems like that may be a feature not available in the Expo Client app. But I think having a functioning Facebook Login is more critical than access to the test users feature in the expo client app

cruzach added 3 commits April 20, 2020 17:07

[ios][facebook] allow Facebook login in Expo Client app

359900f

update docs

fe16580

changelog entry

a81ae57

cruzach requested a review from sjchmiela April 20, 2020 21:24

sjchmiela reviewed Apr 22, 2020

View reviewed changes

cruzach added 5 commits April 22, 2020 18:12

fix docs: no need to go to FB app settings

5c0a21e

Log when overriding appId in client

4a98fd8

check for facebookAppId in case in custom client

8aed626

remove method swizzling

000b7df

make expokit lowest priority subcontractor

0ec1ef0

cruzach requested review from mczernek and tsapeta as code owners April 23, 2020 02:51

cruzach changed the title ~~[ios][facebook] allow Facebook login in Expo Client~~ [ios][facebook] add schemes for facebook login & always handle openURL in EXFacebookAppDelegate Apr 23, 2020

cruzach requested a review from sjchmiela April 23, 2020 03:08

sjchmiela reviewed Apr 23, 2020

View reviewed changes

cruzach added 3 commits April 23, 2020 11:29

make 'priority' a method, returning NSInteger

1c8fab8

pr feedback: conditional logging & docs

d64be15

backport changes to versioned SDK 37

0cb9033

sjchmiela approved these changes Apr 23, 2020

View reviewed changes

cruzach added 3 commits April 23, 2020 16:58

revert changes to LSApplicationQueriesSchemes in info.plist

14d4a39

update changelog, add breaking change

499f6bd

update display name in info.plist

2355a2d

cruzach merged commit b5c35b9 into master Apr 24, 2020

cruzach deleted the @cruzach/fb-login-client branch April 24, 2020 18:31

cruzach added a commit that referenced this pull request Apr 24, 2020

[ios][facebook] add schemes for facebook login & always handle openUR…

fd15f8e

…L in EXFacebookAppDelegate (#7931)

cruzach mentioned this pull request May 16, 2020

Problem login with Facebook and Firebase #8226

Closed

cruzach mentioned this pull request Aug 18, 2020

Branch.subscribe listeners and calls to Branch.getLatestReferringParams(true) do not behave all times correctly on iOS #6568

Closed

		@@ -6,6 +6,8 @@ NS_ASSUME_NONNULL_BEGIN

		@interface UMSingletonModule : NSObject

		@property (nonatomic) int priority;

[ios][facebook] add schemes for facebook login & always handle openURL in EXFacebookAppDelegate #7931

[ios][facebook] add schemes for facebook login & always handle openURL in EXFacebookAppDelegate #7931

Uh oh!

Conversation

cruzach commented Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

How

Test Plan

Uh oh!

sjchmiela left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sjchmiela Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

cruzach Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cruzach commented Apr 23, 2020

Uh oh!

sjchmiela left a comment

Choose a reason for hiding this comment

Uh oh!

sjchmiela Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sjchmiela Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sjchmiela Apr 23, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sjchmiela left a comment

Choose a reason for hiding this comment

Uh oh!

appsgenie commented Apr 29, 2020

Uh oh!

jeanlucnguyen commented May 6, 2020

Uh oh!

atyrian commented May 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cruzach commented May 11, 2020

Uh oh!

Uh oh!

cruzach commented Apr 20, 2020 •

edited

Loading

atyrian commented May 9, 2020 •

edited

Loading