DialogModule supports only FragmentActivity

[dulmandakh]

Summary

Now RN has only ReactActivity which extends AppCompatActivity, subclass of FragmentActivity, therefore no need to check if activity is FragmentActivity or not. This PR changes DialogModule to work only with FragmentActivity.

Also DialogFragment from Android is deprecated in API 28, and recommends to use DialogFragment from Support Library. Excerpt from DialogFragment documentation.

This class was deprecated in API level 28.
Use the Support Library DialogFragment for consistent behavior across all devices and access to Lifecycle.

BREAKING CHANGE: Brown field apps must extend FragmentActivity or its subclasses.

Changelog

[Android] [Changed] - DialogModule supports only FragmentActivity

Test Plan

CI is green, and works as usual.

[dulmandakh]

@mdvacca could you please look at test failure?

[hey99xx]

Assuming DialogModule would only be used by new RN apps is a wrong assumption. Casting getCurrentActivity() to ReactActivity would mean that any existing activity that is not a subclass of ReactActivity in brownfield apps would no longer able to use a RN dialog.

[dulmandakh]

@hey99xx changed to use AppCompatActivity, but DialogModuleTest is failing. I have very little experience with Java tests. Could you please help with this?

[hey99xx]

I still think this PR is an issue. In my company we have a CompanyReactActivity which displays ReactRootViews in fragments. CompanyReactActivity extends from CompanyActivity that has a bunch of company specific onCreate, onDestroy etc. code, and that extends from FragmentActivity. We have never needed to use AppCompatActivity to host RN, and I don't see why the switch is even necessary.

[dulmandakh]

@hey99xx I think that it's safe to drop plain Activity support

[hey99xx]

While you could obviously cast to FragmentActivity and resolve my worry, another brownfield app may be using a plain Activity and this would still be an issue for them.

I think the intention behind allowing multiple fragment types was beneficial to integrating RN with brownfield apps, I don't know why you'd want to drop that now.

[dulmandakh]

@hey99xx Google recommends to extend AppCompatActivity for Android apps, and thought that it's better to drop support for plain Activity. Thanks for you suggestions, I don't have experience with brownfield apps. Now, it uses FragmentActivity.

[dulmandakh]

@hey99xx I'll be requesting your review when I make such changes. Thank you

[hey99xx]

Btw I think some of RN widgets are not ready to extend from AppCompat classes in couple line changes, they cause runtime issues on old Android versions. I've described what goes wrong in #22885 and made a comment in one of your other PRs.

[dulmandakh]

@hey99xx I saw the issue, and though that it would be best to have only FragmentActivity and it's subclasses supported, then remove plain Activity support. Therefore, change how contexts works. Otherwise, we'll have duplicate code to support both FragmentActivity and plain Activity which doubles error surface. Please correct me if wrong.

[dulmandakh]

Also found that DialogFragment is deprecated in API 28, and recommends to use DialogFragment from Support Library, which this PR does.

[facebook-github-bot]

@mdvacca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[mdvacca]

The land failed because there is a test that is using activity and can not be casted to FragmentActivity:

java.lang.ClassCastException: android.app.Activity cannot be cast to android.support.v4.app.FragmentActivity
	at com.facebook.react.modules.dialog.DialogModule.getFragmentManagerHelper(DialogModule.java:244)
	at com.facebook.react.modules.dialog.DialogModule.onHostResume(DialogModule.java:176)
	at com.facebook.react.bridge.ReactContext$1.run(ReactContext.java:159)

[mdvacca]

This will be a breaking change, should we deprecate? communicate about this change and then change this code?

what do you think @cpojer , @hramos ?

[cpojer]

@mdvacca how many people do you think this will be breaking for? If the answer is at most ~8%, then I'd say let's ship this change now and ask them to upgrade.

[cpojer]

@mdvacca if it works at FB, let's land it and move forward. We need to do this eventually anyway.

[cpojer]

@dulmandakh can you rebase this?

[cpojer]

@dulmandakh seems like this is failing android CI. Can you fix?

[cpojer]

Let's see if this works.

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[cpojer]

@dulmandakh we are getting Summary: No build file at third-party/android/support/v7/appcompat-orig/BUCK when resolving target //third-party/android/support/v7/appcompat-orig:appcompat., so it seems we don't have that target.

[dulmandakh]

yep, it's AndroidX now. I'll rebase and fix the issue. Almost forgot this PR 😄

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @dulmandakh in 243070a.

^{When will my fix make it into a release? | Upcoming Releases}