Skip to content
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

Implemented expo plugin #111

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jexodusmercado
Copy link

Added expo plugin to modify AppDelegate with development builds

Note: I haven't included any test to check the generated AppDelegate. Currently works with my setup. Let me know if you have encountered any issues

@ruslanjabari
Copy link

God bless.

@ruslanjabari
Copy link

@jexodusmercado

issue: when updating the callerName via RN, the underlying callerName still retains "Connecting..."

This happens because of this line:
NSString *callerName = [NSString stringWithFormat:@"%@ (Connecting...)", payload.dictionaryPayload[@"callerName"]];"

modResults.contents = modResults.contents.replace(
/#import "AppDelegate.h"/g,
`#import "AppDelegate.h"
#import <PushKit/PushKit.h>
Copy link

Choose a reason for hiding this comment

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

as a suggestion, there is a function in expo plugin to handle imports.

const { addObjcImports } = require('@expo/config-plugins/build/ios/codeMod');

Then, you can use it like this.

modResults.contents = addObjcImports(modResults.contents, ['#import <PushKit/PushKit.h>', '#import "RNVoipPushNotificationManager.h"', '#import "RNCallKeep.h"'])

Copy link

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

This should be a comment, not a review.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @AZReed. Kinda confuse about this. Should I proceed making this changes you suggest?

Choose a reason for hiding this comment

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

That's the problem with reviews. It's hard to tell of someone is just flexing or not. They say "suggestion" but open a review.

Copy link

Choose a reason for hiding this comment

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

Hey @jexodusmercado
Maybe it should have been a comment, not sure, my apologies for the confusion. I press the comment option in the review, not the request changes, I though that was clear enough.

It's your time and you already did enough, so it's up to you.

Thanks for your work by the way, it's awesome!

@wilkinsonj
Copy link

wilkinsonj commented Jul 4, 2024

Hey guys, LOVE this, its badly needed.

Suggestion: the 'video' property (YES or NO) should be based on the payload (like UUID, Handle etc). Otherwise there's no way to initiate a video call (which is important as it opens the app straight away once the call is accepted, and also appears in the call description)

And on that note, I think all properties should be configurable from payload, and have defaults (if not provided in the payload). I spent a lot of time working out why the app was crashing due to hard coded expectation of variables in the payload.

For example, handle could be 'Unknown caller' if no handle property is in the data dictionary. Video could default to YES.

@ice-cap0
Copy link

yes please

…n-video calls. remove merging invocation block due to authentication issues whennot initiating inside root.
@AlanGreyjoy
Copy link

AlanGreyjoy commented Aug 17, 2024

@ianlin Can we get this on a track for merging soon?

Copy link

@AZReed AZReed left a comment

Choose a reason for hiding this comment

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

🚀

@jexodusmercado
Copy link
Author

@zxcpoiu can you check and merge this if applicable? Thanks!

@southkeys
Copy link

what's the hold up?

@zxcpoiu
Copy link
Member

zxcpoiu commented Nov 22, 2024

Hi @jexodusmercado thanks for the work, and the late respond.

I'm not using expo, so not familiar the plugin structure and magics.

There is another expo support PR discussion at react-native-webrtc/react-native-webrtc#1013 and react-native-webrtc/react-native-webrtc#1014 which uses Expo Config Plugin.

At a glance I have few points as follow:

  1. I don't have enough knowledge and time to maintain expo version compatibility from time to time
  2. Not every one use expo, and this add unnecessary dependencies for non-expo users
  3. The PR seems to duplicate the whole appDelegate code into the expo plugin file, does it mean we have to change more than one file when modifying appDelegate related code?

Wouldn't it better to create another repo like: react-native-voip-push-notification-expo?
The new repo can add this repo as dependency and just add the expo specific customization, like: read and inject/re-generate appDelegate codes, custom gitignore and npm ignore...etc.

And we can add expo user guide in README redirect to the expo repo.

Sound good?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants