-
Notifications
You must be signed in to change notification settings - Fork 263
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
fix: add check to avoid crashes when currentActivity is null #846
fix: add check to avoid crashes when currentActivity is null #846
Conversation
android/src/main/java/com/reactnativestripesdk/StripeSdkModule.kt
Outdated
Show resolved
Hide resolved
@@ -172,7 +172,9 @@ class StripeSdkModule(private val reactContext: ReactApplicationContext) : React | |||
private val googlePayReceiver: BroadcastReceiver = object : BroadcastReceiver() { | |||
override fun onReceive(context: Context?, intent: Intent) { | |||
if (intent.action == ON_GOOGLE_PAY_FRAGMENT_CREATED) { | |||
googlePayFragment = (currentActivity as AppCompatActivity).supportFragmentManager.findFragmentByTag("google_pay_launch_fragment") as GooglePayFragment | |||
(currentActivity as? AppCompatActivity)?.let { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was really surprised that you have to check the current activity for null everywhere, even from @ReactMethod, but I didn't really see another way.
In order to prevent this type of error in the future I would recommend:
class StripeSdkModule ...{
...
// Set currentActivity so it uses this instead of the base class `getCurrentActivity()`
// this will give you type safety checking and convert it to an appCompat activity
val currentActivity: AppCompatActivity?
get() = (super.getCurrentActivity() as? AppCompatActivity)
...
private val googlePayReceiver: BroadcastReceiver = object : BroadcastReceiver() {
override fun onReceive(context: Context?, intent: Intent) {
if (intent.action == ON_GOOGLE_PAY_FRAGMENT_CREATED) {
currentActivity?.let {
...
}
It would also be nice to see some enforcement in the code that if you can't get the appCompat activity that a promise is fired to return a result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the feedback! created a helper method for getting the activity or resolving the promise. It's not the perfect solution since someone can still call currentActivity
, but it does avoid a lot of repeated code and hopefully anyone working here would see the current pattern for getting the activity 🤞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michelleb-stripe do you approve the most recent changes? I'll need to rebase but since #879 contains substantial changes to the same file, i'd rather wait till after that lands to rebase (to avoid duplicating the work)
closes #881 |
d11edcb
to
369587b
Compare
369587b
to
39a63e1
Compare
Summary
We weren't always appropriately null-checking
currentActivity
, and the typecast could result in crashes. We now resolve with a consistent and useful error telling the user they can retry the method.Motivation
closes #844 and I'm sure other instances of crashes on Android
closes #881
Testing
Documentation
Select one: