-
Notifications
You must be signed in to change notification settings - Fork 88
Refactor / Simplify Codes, support didLoadWithEvents #69
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
Conversation
* remove unused codes, keep this lib simple, lots functionalities should be easily implemented in the modern RN eco system - remove requestPermission ( ican use PushNotificationIOS ) - remove getting background state ( can use AppState ) * use NativeEventEmitter instead the deprecated DeviceEventEmitter * support caching events before js initialize you can subscribe `didLoadWithEvents` when app ready
Usage:We have 2 API:
And 3 Events:
...
import VoipPushNotification from 'react-native-voip-push-notification';
...
class MyComponent extends React.Component {
...
// --- or anywhere which is most comfortable and appropriate for you, usually ASAP
componentDidMount() {
VoipPushNotification.registerVoipToken(); // --- register token
VoipPushNotification.addEventListener('didLoadWithEvents', (events) => {
// --- this will fire when there are events occured before js bridge initialized
// --- use this event to execute your event handler manually by event type
if (!events || !Array.isArray(events) || events.length < 1) {
return;
}
for (let voipPushEvent of events) {
let { name, data } = voipPushEvent;
if (name === VoipPushNotification.RNVoipPushRemoteNotificationsRegisteredEvent) {
this.onVoipPushNotificationRegistered(data);
} else if (name === VoipPushNotification.RNVoipPushRemoteNotificationReceivedEvent) {
this.onVoipPushNotificationiReceived(data);
}
}
});
// --- onVoipPushNotificationRegistered
VoipPushNotification.addEventListener('register', (token) => {
// --- send token to your apn provider server
});
// --- onVoipPushNotificationiReceived
VoipPushNotification.addEventListener('notification', (notification) => {
// --- when receive remote voip push, register your VoIP client, show local notification ... etc
this.doSomething();
// --- optionally, if you `addCompletionHandler` from the native side, once you have done the js jobs to initiate a call, call `completion()`
VoipPushNotification.onVoipNotificationCompleted(notification.getData().uuid);
});
}
// --- unsubscribe event listeners
componentWillUnmount() {
VoipPushNotification.removeEventListener('didLoadWithEvents');
VoipPushNotification.removeEventListener('register');
VoipPushNotification.removeEventListener('notification');
}
...
} |
e447e54
to
46bb6ee
Compare
7617392
to
ddefd9d
Compare
The PushKit require us to REGISTER delegate first, then the PushKit obj is responsible to pass Without register delegate, we will not receive any callback from PushKit, so it is recommended to register voip ASAP in |
I have exposed readme updated as well. |
Usage to register voip push in AppDelegate.m ASAP - (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions
{
RCTBridge *bridge = [[RCTBridge alloc] initWithDelegate:self launchOptions:launchOptions];
// ===== (THIS IS OPTIONAL) =====
// --- 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];
// ===== (THIS IS OPTIONAL) =====
RCTRootView *rootView = [[RCTRootView alloc] initWithBridge:bridge moduleName:@"AppName" initialProperties:nil];
} |
I will leave it here few days for folks to test and feedback. Then merge into master with 3.0 version. |
@zxcpoiu Just 2 remarks, don't know if it's the expected behavior:
env: thanks |
HI @Jerome91410 , thanks for the feedback.
So it does NOT mean the If the JS bridge is initialized earlier, the event will fires through the normal event handler. Personally I would suggest:
All in allEach event should fire once, either via |
Thanks for the explanation. It's exactly what I did. The package is initialized in I'll double check, but as the app is started (previously killed) due to a push voip received, the event should be automatically fired before the JS initialization. |
@zxcpoiu I doubled check. I did some changes; and now I am more confident to get the token (if requested from native part) and the previous notification with this kind of code: diff --git a/node_modules/react-native-voip-push-notification/index.js b/node_modules/react-native-voip-push-notification/index.js
index 88949f1..c5f730b 100644
--- a/node_modules/react-native-voip-push-notification/index.js
+++ b/node_modules/react-native-voip-push-notification/index.js
@@ -95,6 +95,31 @@ export default class RNVoipPushNotification {
RNVoipPushNotificationManager.registerVoipToken();
}
+ /**
+ * usefull to get the last register token if your JS takes time to initialize the listener on `didLoadWithEvents`
+ * Should be call before events initialization
+ *
+ * @static
+ * @memberof RNVoipPushNotification
+ *
+ *
+ */
+ static getLastVoipToken() {
+ return RNVoipPushNotificationManager.getLastVoipToken();
+ }
+
+ /**
+ * usefull to get the last notification if your JS takes time to initialize the listener on `didLoadWithEvents`
+ * Should be call before events initialization
+ *
+ * @static
+ * @memberof RNVoipPushNotification
+ *
+ */
+ static getLastNotification() {
+ return RNVoipPushNotificationManager.getLastNotification();
+ }
+
/**
* When you have processed necessary initialization for voip push, tell ios completed.
* This is mainly for ios 11+, which apple required us to execute `complete()` when we finished.
diff --git a/node_modules/react-native-voip-push-notification/ios/RNVoipPushNotification/RNVoipPushNotificationManager.m b/node_modules/react-native-voip-push-notification/ios/RNVoipPushNotification/RNVoipPushNotificationManager.m
index 2699a77..4ee9130 100644
--- a/node_modules/react-native-voip-push-notification/ios/RNVoipPushNotification/RNVoipPushNotificationManager.m
+++ b/node_modules/react-native-voip-push-notification/ios/RNVoipPushNotification/RNVoipPushNotificationManager.m
@@ -30,7 +30,8 @@ @implementation RNVoipPushNotificationManager
static bool _isVoipRegistered = NO;
static NSMutableDictionary<NSString *, RNVoipPushNotificationCompletion> *completionHandlers = nil;
-
+static NSString *_lastVoipToken = nil;
+static NSDictionary *_lastNotification = nil;
// =====
// ===== RN Module Configure and Override =====
@@ -160,9 +161,10 @@ + (void)didUpdatePushCredentials:(PKPushCredentials *)credentials forType:(NSStr
for (NSUInteger i = 0; i < voipTokenLength; i++) {
[hexString appendFormat:@"%02x", bytes[i]];
}
-
RNVoipPushNotificationManager *voipPushManager = [RNVoipPushNotificationManager allocWithZone: nil];
[voipPushManager sendEventWithNameWrapper:RNVoipPushRemoteNotificationsRegisteredEvent body:[hexString copy]];
+ _lastVoipToken =[hexString copy];
+
}
// --- should be called from `AppDelegate.didReceiveIncomingPushWithPayload`
@@ -171,9 +173,9 @@ + (void)didReceiveIncomingPushWithPayload:(PKPushPayload *)payload forType:(NSSt
#ifdef DEBUG
RCTLog(@"[RNVoipPushNotificationManager] didReceiveIncomingPushWithPayload payload.dictionaryPayload = %@, type = %@", payload.dictionaryPayload, type);
#endif
-
RNVoipPushNotificationManager *voipPushManager = [RNVoipPushNotificationManager allocWithZone: nil];
[voipPushManager sendEventWithNameWrapper:RNVoipPushRemoteNotificationReceivedEvent body:payload.dictionaryPayload];
+ _lastNotification =payload.dictionaryPayload;
}
// --- getter for completionHandlers
@@ -212,6 +214,28 @@ + (void)removeCompletionHandler:(NSString *)uuid
});
}
+
+// --- get last voip push token
+RCT_REMAP_METHOD(getLastVoipToken,
+ resolverToken:(RCTPromiseResolveBlock)resolve
+ rejecterToken:(RCTPromiseRejectBlock)reject)
+{
+#ifdef DEBUG
+ RCTLog(@"[RNVoipPushNotificationManager] getLastVoipToken() _lastVoipToken = %@", _lastVoipToken);
+#endif
+ resolve(_lastVoipToken);
+ _lastVoipToken = nil;
+}
+
+// --- get last voip push token
+RCT_REMAP_METHOD(getLastNotification,
+ resolverNotif:(RCTPromiseResolveBlock)resolve
+ rejecterNotif:(RCTPromiseRejectBlock)reject)
+{
+ resolve(_lastNotification);
+ _lastNotification = nil;
+}
+
// --- called from js when finished to process incoming voip push
RCT_EXPORT_METHOD(onVoipNotificationCompleted:(NSString *)uuid)
{ how i use it: const lastNotification = await VoipPushNotification.getLastNotification();
if (lastNotification) {
... // Do what you want with
}
VoipPushNotification.addEventListener('didLoadWithEvents', events => {
...
})
VoipPushNotification.addEventListener('notification', notification => {
...
}) |
Thanks @Jerome91410 Yeh, I got your point and have considered it while I made this PR. This is more like a confidence level to the js event bridge. The principle of this lib is to propagate native events to the js reliably and brainlessly, said, even if it is a duplicated register event from PushKit or when multiple VoipPush received at the same time, we should propagate it to the js for user to handle. Honestly, I personally like the reliable feel of your solution ( invoke and get response right away instead waiting events without any indicator ), but it can't handle the multiple voip notification arrived at the same time since you only have on variable to store it. Another potential race condition ( or duplicate events confusion ) of your solution may occur like below: const lastNotification = await VoipPushNotification.getLastNotification();
if (lastNotification) {
... // Do what you want with
}
// =====
// Race Condition and risk for duplicated events here
//
// after we check the initial event, whether it is exist or not,
// at this moment, we received another event,
// but while native `startObserving` is not being called yet and `_hasListeners` is still `NO`,
//
// We will not only save the last event to `_lastNotification` / `_lastVoipToken` but also add it to the cached `_delayedEvents`.
//
// So eventually, we still relied on `didLoadWithEvents`
// or we can call `getLastNotification()` / `getLastVoipToken()`
// again after we've subscribed event listeners. ( But we won't
// know when `startObserving` is actually being called in js side
// until we got the first event )
//
// And we have to check / distinguish whether the events from `didLoadWithEvents` are duplicated with `getLastNotification` / `getLastVoipToken`
//
// To harmonize the above issue, we will increase complexity to this lib or our app
//
// =====
VoipPushNotification.addEventListener('didLoadWithEvents', events => {
...
}) So if your use case is really that time sensitive which can't wait for the event bridge, you can made another PR on top of this one to add the method, I am happy to merge it but would not recommend it in the doc. Thanks for sharing it. 👍 |
@zxcpoiu .. We tested this branch with iOS and everything is working flawlessly on our end. Specifically, it has successfully fixed VOIP notifications showing while the app is in background/killed states and successfully fires the 'notification' callback so we can register our websocket. Thank you for your contribution! |
Does this allow both Android + IOS open app when terminated, or just IOS? Is it possible for Android too? Also how do you register that the incoming call has hanged up? E.g: Call user -> user phone is off -> cancel call -> user turns phone on -> user opens app -> incoming call (already been cancelled on other side). How do you fix this? |
This lib is for iOS only, entirely nothing to do with android.
There is nothing to fix but how your app use Voip Push For cancel a expired voip push, usually:
|
Hi for some reason my registration callback has stopped firing. I have tried with both
and
My version is:
It was working before, but now 'runs 3' never gets console.logged: console.warn('runs');
if (Platform.OS === 'ios') {
console.warn('runs2');
// VoipPushNotification.registerVoipToken();
VoipPushNotification.addEventListener('register', (token: string) => {
console.warn('runs3');
PushNotification.configure({
onRegister: onRegister(token),
onNotification,
popInitialNotification: true,
requestPermissions: true,
alert: true,
badge: true,
sound: true,
} as any);
}); |
You can do a simple experiment:
If you see the register event, then the issue you mentioned is likely a |
When I press the button I don't get 'register' callback. But this This is what I get in Xcode Console when I press the button: So 'register' event never runs in my JS code, and I don't know what the APNS_VOIP token is AppDelegate.m for reference. Also, thank you for helping and this library. It was amazing when it worked for me. I'm just not sure why it's stopped. |
You have called You should use BOTH: didLoadWithEvents and your normal register events. Please check this example out: |
This worked, thank you! |
@zxcpoiu, I like your way of solving voip push notifications - clean and nice. |
First, this solution is originally provided from @konradkierus at react-native-webrtc/react-native-callkeep#169, there are some discussion similar your question react-native-webrtc/react-native-callkeep#169 (comment) My thinking path when I'm writing this PR was simply:
Personally, I prefer to hide |
I don't see any contradiction with @konradkierus. When we will remove didLoadWithEvents, we will keep the order of events based on the index in queue. So that will also quarantie the order and we would have full control over events. |
May be I didn't understand what is your point. |
Is above what you mean? |
Yep, I suggest to remove didLoadWithEvents entirely as it is unnecessary and simplifies the code. |
Disagree entirely. I use 'didLoadWithEvents' And I don't even need 'register'. Register event gets called in Native code and my app never sees event as it always runs first (in AppDelegate.m). So my app Always Loads with event using 'didLoadWithEvent' and this is used to set the APNS VOIP token to async storage and the app then uses token to send to VOIP Backend to find and call phone. 'didLoadWithEvents' is very useful. |
@stephanoparaskeva Do you know that didLoadWithEvents fire both register and notification events in one array? If you don't need 'register' than you don't have to subscribe to it. Probably you do not get the idea here. So you will just have to listen 'register' in your app and when get one - use received voip token. That would be possible only after suggested changes. And just remove unneeded 'didLoadWithEvent'. By the way how do you handle voip token change event? |
I honestly think it's fine as is. 'didLoadWithEvents' gives you all events executed before JS code runs, so you can have events first before everything else. It's quite clever.
I don't handle this event, should I, and what for? |
Hey, sorry for late reply, and happy new year. Actually @Romick2005 is right, we can of course remove
So I prefer to not introduce another breaking change again, unless it's necessary and the change can compatible with both method ( ex: while implementing multiple separated queue for more intuitive api like @Romick2005 said and support |
@zxcpoiu Please should this be used alongside the native configurations in the AppDelegate.m
then is my class that exports voip listeners
then in my App.js I import
Please I can get this to work. My expected behaviour
I cant seem to when this to work. please an help will do |
Breaking Change
This PR does the following:
remove unused codes, keep this lib simple, lots functionalities should be easily implemented in the modern RN eco system
PushNotificationIOS
instead )AppState
instead )use
NativeEventEmitter
instead the deprecatedDeviceEventEmitter
tweak event handler logic
support caching events before js initialize
you can subscribe
didLoadWithEvents
when app readyAbout didLoadWithEvents
This is aim to fix #59 ( voip push received and the event fired before js initialized )
@JJMoon proposed a workaround like in PR #65 and a proper fix in PR #61
Thanks!
But then I tend to use a different way which is the same with react-native-webrtc/react-native-callkeep/pull/205
The reason:
Instead using a promise method to get events directly, using event based with
startObserving
, we can make sure the bridge is up and READY for events, and this may prevent edge case between get initial events and subsequently fired events.We cache events if js is not up, this can be work together with, if you would like to use
voipRegistration
directly indidFnishLaunchingWithOptions
inAppDelegate.m
. This is described in didReceiveIncomingPushWithPayload not called in background/terminated #59 (comment) by @chevonc . Not sure but I believe that helps in some situation. (We don't know how exactly PushKit works and when will it DELIVER voip push to us happily.)Side question and discussion
The thing I can not figure out is, when app killed, and the app can be wake up by the remote push and enter the
didFnishLaunchingWithOptions
, doesn't that mean we already have a valid voip token to RECEIVE voip push?Does PushKit invoke
didReceiveIncomingPushWithPayload
only when we have registered voip token?Do we recommend to move
voipRegistration
indidFnishLaunchingWithOptions
inAppDelegate.m
?