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

Move to RCTEventEmitter, unified JS initialization and more. #395

Closed

Conversation

krystofcelba
Copy link

@krystofcelba krystofcelba commented Feb 17, 2018

Hi,
I have recently needed to bring support for OneSignal to RN app which I'm working on and logically the first thing what I tried was this library - react-native-onesignal. Unfortunately, I was facing many issues, that was related to the setup of my app where I use for example react-native-navigation or redux-saga. The other thing is that the react-native-onesignal has many things implemented in way that is very different to that we can see in native onesignal library for iOS and Android and some things aren't implemented at all. I think that it's not good as on one side it's confusing for someone who wants to use this library. On the other side, it has to be very difficult to maintain it, particularly when there is some change in the native one's libs.
So for that reasons I have decided to look under the hood and try to solve some of that. Now I would like to show off what I have done for now and hear what do you think about that. Eventually, I will be more than happy to collaborate on merging it.

So now something about the changes.

  1. RCTOneSignal class on iOS now inherits from RCTEventEmitter. Thanks to that it's not needed to use the deprecated way to interact with JS.
  2. The initialization of OneSignal which was solved by the line that has to be copied to AppDelegate gets impossible that way, as the subclasses of RCTEventEmitter are instantiated by react-native. So I was finding some way how to do it differently and fortunately, I found that in the OneSignal/OneSignal-Cordova-SDK they had to solve something similar. After some adjustments, it can be used in our case too. Here I wrote some details about that.
  3. The result is a unified way to set up the library on both iOS and Android. Here is example: OneSignal.init(Constants.oneSignalAppId, { autoPrompt: true }); this line have to be added to the main index.js of your app and it's all. No need to change something in Xcode or Android studio 🙂.
  4. Most of the functions return promise now. This is mainly advantaged on iOS because the implementation of native SDK for Android is different in the promise gets resolved instantly in most cases here despite the of real result. But I found it useful anyway, async/await syntax can be used now etc..
  5. There shouldn't be any issues with initial notifications etc. as that events get holds on the native side even if they were dispatched and the JS context isn't initialized yet. They will be dispatched right to the appropriate handler for that event is registered on JS side.
  6. Most of the functions now directly call that in native SDKs. So the developer experience should be unified with official SDKs on other platforms.
  7. I have added TypeScript definitions file.

It still needs some work (documentation, ..) but I believe that the changes are meaningful.

Thanks.
PS: Sorry for the awkward title of PR.

This change is Reviewable

 * RCTOneSignal now overrides RCTEventEmitter
 * The initialization with app id is now done from JS - same way on both platforms. So it's not needed to edit AppDelegate anymore.
 * Overall refactoring and cleanup. Now it's more like simple bridge to native SDK so the maintaining will be easier.
 * Should work with redux/redux-saga without issues as well as with wix/react-native-navigation.
 * The initialization with app id is now done from JS - same way on both platforms. So it's not needed to add manifect_placeholders to build.gradle etc.
 * Overall refactoring and cleanup. Now it's more like simple bridge to native SDK so the maintaining will be easier.
 * Should work with redux/redux-saga without issues as well as with wix/react-native-navigation.
 * Added `init` method for initting OneSignal from JS.
 * The most functions now return promises.
 * Simplified registering of native events listeners.
@Nightsd01
Copy link
Contributor

Thanks for your hard work, we’ll take a look at this PR!

@Nightsd01
Copy link
Contributor

@krystofcelba we are all very impressed with your work and we'd love to use it. Unfortunately this PR has a lot of breaking changes that fundamentally change the way the SDK works.

Most of your changes are definitely good changes that we want to implement, but at this time we're not ready to change the SDK in such a large way. We are releasing an update for the react-native SDK and we've actually been working on some of the same things you did in this PR (ie. switching to an RCTEventEmitter subclass) but we wanted to do it in a way that is backwards compatible.

In the next major release, we will definitely be using much of your PR. Thank you for your excellent work!

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.

None yet

3 participants