search

hoxfon / react-native-twilio-programmable-voice

React Native wrapper for Twilio Programmable Voice SDK
MIT License
181 stars 152 forks source link

feat: configureCallKit before JS initialisation to avoid dropping incoming calls on cold start #193

Closed vuletuanbt closed 3 years ago

vuletuanbt commented 3 years ago 

- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions
{
 ...
  RCTBridge *bridge = [[RCTBridge alloc] initWithDelegate:self launchOptions:launchOptions];
  _rnTwilioVoice = [bridge moduleForClass:[RNTwilioVoice class]];
  [_rnTwilioVoice initPushRegistry];
  [_rnTwilioVoice reRegisterWithTwilioVoice];

    NSDictionary *configCallkit = @{@"appName": @"AppName"};
  [_rnTwilioVoice configCallKit:configCallkit];

  // ---  Voip Push Notification
  // ===== (THIS IS OPTIONAL BUT RECOMMENDED) =====
  // --- register VoipPushNotification here ASAP rather than in JS. Doing this from the JS side may be too slow for some use cases
  // --- see: https://github.com/react-native-webrtc/react-native-voip-push-notification/issues/59#issuecomment-691685841
  [RNVoipPushNotificationManager voipRegistration];
  ...
  return YES;
}
jdegger commented 3 years ago

Thank you for your PR @vuletuanbt, we will review it as soon as possible!

@fabriziomoscon For review

fabriziomoscon commented 3 years ago

@vuletuanbt I will test this tonight

fabriziomoscon commented 3 years ago

@vuletuanbt I will continue testing this today, as last night I run into Signing certificates issues

fabriziomoscon commented 3 years ago

UPDATE: @vuletuanbt I am currently installing the latest Xcode as I need it to test iOS 14

fabriziomoscon commented 3 years ago

UPDATE: @vuletuanbt with Xcode 12.4 my app doesn't build anymore, so before I can test your code I need to fix this issue.

rkrk9 commented 3 years ago

@fabriziomoscon @vuletuanbt I build successful with the following code.

- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions
{
  RCTBridge *bridge = [[RCTBridge alloc] initWithDelegate:self launchOptions:launchOptions];

  id _rnTwilioVoice = [bridge moduleForClass:[RNTwilioVoice class]];

  [_rnTwilioVoice initPushRegistry];
  [_rnTwilioVoice reRegisterWithTwilioVoice];
fabriziomoscon commented 3 years ago

UPDATE: I updated Xcode to 12.4, RN to 0.62.2 and fixed the iOS build issues. Next step for me would be to use these changes and test that I can receive calls in background

fabriziomoscon commented 3 years ago

@vuletuanbt please could you let me know which pod TwilioVoice do you use?

vuletuan commented 3 years ago

@vuletuanbt please could you let me know which pod TwilioVoice do you use?

pod 'TwilioVoice', '~> 5.2.0' It's your package default. What's your problem ?

fabriziomoscon commented 3 years ago

@vuletuan after reading these: https://github.com/react-native-webrtc/react-native-voip-push-notification/issues/59 https://github.com/react-native-webrtc/react-native-voip-push-notification/pull/69 I understood the intention of your PR.

When receiving a background call, there could be a race condition between the native handling of the push and the JS initialisation: native handling push notification: ====> 1ms JS callkit initialisations: ============> 2.3 ms that results in the call being not reported. This issue is also present in Android and I am already working on a PR containing the fix: https://github.com/hoxfon/react-native-twilio-programmable-voice/pull/164#issuecomment-770374714

To fix this issue this PR should suggest to perform as much of the CallKit initialisation as possible in the main app.

vuletuanbt commented 3 years ago

@vuletuan after reading these: react-native-webrtc/react-native-voip-push-notification#59 react-native-webrtc/react-native-voip-push-notification#69 I understood the intention of your PR.

When receiving a background call, there could be a race condition between the native handling of the push and the JS initialisation: native handling push notification: ====> 1ms JS callkit initialisations: ============> 2.3 ms that results in the call being not reported. This issue is also present in Android and I am already working on a PR containing the fix: #164 (comment)

To fix this issue this PR should suggest to perform as much of the CallKit initialisation as possible in the main app.

So, you mean I could use an event to communicate with JS instead of the directed way like I did to handle the incoming call

fabriziomoscon commented 3 years ago

No, I will integrate the necessary changes into #164

fabriziomoscon commented 3 years ago

@vuletuanbt I have added this commit on top of the new branch: https://github.com/hoxfon/react-native-twilio-programmable-voice/pull/164/commits/c176602f99907bf6d301a0d0ea9308e26eac8cc0 feel free to test it.

But it doesn't work for me, because the to register for CallKit you need the accessToken which is provided by the server, which is contacted by JS, where the user session credentials are stored.

I see this errors in the Xcode debug log:

ERROR:Twilio:[Platform](0x102d46bc0): Registration failed: Error Domain=com.twilio.voice.error Code=31301 "Registration failed" UserInfo={NSLocalizedDescription=Registration failed, NSLocalizedFailureReason=Invalid Access Token}
2021-04-07 00:12:39.225224+0100 Hoxfon[336:12211] ERROR:Twilio:[Platform](0x102d46bc0): Inside register:deviceToken:completion:, failed to register for Twilio push notifications. Error:Registration failed
2021-04-07 00:12:39.227025+0100 Hoxfon[336:12211] An error occurred while registering: Registration failed
2021-04-07 00:12:39.228 [warn][tid:main][RCTEventEmitter.m:53] Sending `deviceNotReady` with no listeners registered

Are you and @reikireirikei experiencing the same issue? How did you make it work?

vuletuan commented 3 years ago

@vuletuanbt I have added this commit on top of the new branch: c176602 feel free to test it.

But it doesn't work for me, because the to register for CallKit you need the accessToken which is provided by the server, which is contacted by JS, where the user session credentials are stored.

I see this errors in the Xcode debug log:

ERROR:Twilio:[Platform](0x102d46bc0): Registration failed: Error Domain=com.twilio.voice.error Code=31301 "Registration failed" UserInfo={NSLocalizedDescription=Registration failed, NSLocalizedFailureReason=Invalid Access Token}
2021-04-07 00:12:39.225224+0100 Hoxfon[336:12211] ERROR:Twilio:[Platform](0x102d46bc0): Inside register:deviceToken:completion:, failed to register for Twilio push notifications. Error:Registration failed
2021-04-07 00:12:39.227025+0100 Hoxfon[336:12211] An error occurred while registering: Registration failed
2021-04-07 00:12:39.228 [warn][tid:main][RCTEventEmitter.m:53] Sending `deviceNotReady` with no listeners registered

Are you and @reikireirikei experiencing the same issue? How did you make it work?

I ran into it the first time launch app but it's ok. After that, you just init Twilio on JS like before. Then you can receive the call normally. Maybe it needs some enhance

fabriziomoscon commented 3 years ago

I see. The title of this PR is a bit misleading: feat: support for receiving incoming call in background on IOS 14, because the current version of master allows for incoming calls in the background on iOS 14, I actually checked. So may I ask you to change the title to: configureCallKit on native app or something similar.

I have checked the Twilio quickstart project and I can see that it has a few important changes, so I think it is best for me to focus on migrating this module to align with the latest Twilio SDK version before reconsidering this PR.

I hope that you would agree with me that having errors at initialisations is not what we want. Feel free to use your own fork until then

vuletuan commented 3 years ago

@fabriziomoscon

I hope that you would agree with me that having errors at initialisations is not what we want

Sorry for my bad. I was missing to resolve this. I just focused to handle the incoming call in the background.

| The title of this PR is a bit misleading: feat: support for receiving incoming call in background on IOS 14, because the current version of master allows for incoming calls in the background on iOS 14, I actually checked.

Is that your master or Twilio ? Can you share with me?

I have checked the Twilio quickstart project and I can see that it has a few important changes, so I think it is best for me to focus on migrating this module to align with the latest Twilio SDK version before reconsidering this PR.

Is that the 5.5.2 version on IOS ?

fabriziomoscon commented 3 years ago

I will leave my experiment that uses your code here: https://github.com/hoxfon/react-native-twilio-programmable-voice/commits/feat/twilio-android-sdk-5-and-ios-bg-call-exp just in case you need to confirm the errors or try to fix it. I am currently working on top of branch: feat/twilio-android-sdk-5, so please consider making PRs to that branch, if you find a fix.

Background calls on iOS work on the current master commit (and on branch feat/twilio-android-sdk-5) of this repo: https://github.com/hoxfon/react-native-twilio-programmable-voice/commit/fe40b2edfe9e125295a515a791d0e0557cbc32b7 Obviously you need to have the correct push certificate setup in Twilio console to receive calls, and there is difference between debug and release certificates. When your certificates are mixed up, Twilio console (under Programmable Voice) has APN errors usually.

Twilio changelog is here: https://www.twilio.com/docs/voice/voip-sdk/ios/changelog Twilio quickstart commits are here: https://github.com/twilio/voice-quickstart-ios/commits/master

My Apps file: AppDeletagate.m has these lines:

- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions
{
  RCTBridge *bridge = [[RCTBridge alloc] initWithDelegate:self launchOptions:launchOptions];
  RCTRootView *rootView = [[RCTRootView alloc] initWithBridge:bridge
                                                   moduleName:@"Hoxfon"
                                            initialProperties:nil];

  if ([FIRApp defaultApp] == nil) {
    [FIRApp configure];
  }

  rootView.backgroundColor = [[UIColor alloc] initWithRed:1.0f green:1.0f blue:1.0f alpha:1];

  self.window = [[UIWindow alloc] initWithFrame:[UIScreen mainScreen].bounds];
  UIViewController *rootViewController = [UIViewController new];
  rootViewController.view = rootView;
  self.window.rootViewController = rootViewController;
  [self.window makeKeyAndVisible];

  NSArray *allPngImageNames = [[NSBundle mainBundle] pathsForResourcesOfType:@"png" inDirectory:nil];
  for (NSString *imgName in allPngImageNames){
    if ([imgName containsString:@"LaunchImage"]){
      UIImage *img = [UIImage imageNamed:imgName];

      if (img.scale == [UIScreen mainScreen].scale && CGSizeEqualToSize(img.size, [UIScreen mainScreen].bounds.size)) {
        rootView.backgroundColor = [UIColor colorWithPatternImage:img];
      }
    }
  }

  NSLog(@"Twilio Voice Version: %@", [TwilioVoice sdkVersion]);

  return YES;
}

If you want to help with this project please be my guest. And I would really like if you could help with commits on branch feat/twilio-android-sdk-5. My plan is to update both Android and iOS to the latest version of Twilio Voice SDK by following the commit of Twilio engineers here:

Thank you for your help!