Skip to content

Remove support of FragmentActivity and related widgets - #19950

Closed
dulmandakh wants to merge 18 commits into
react:masterfrom
dulmandakh:remove-fragment-activity
Closed

Remove support of FragmentActivity and related widgets#19950
dulmandakh wants to merge 18 commits into
react:masterfrom
dulmandakh:remove-fragment-activity

Conversation

@dulmandakh

@dulmandakhdulmandakh commented Jun 28, 2018

Copy link
Copy Markdown
Contributor

This PR removes support for FragmentActivity, which is used to support Fragments on Android version below API 11, because RN supports Android API 16 and above.

Please review: @hramos@mdvacca

Test Plan:

Everything works just normal

Release Notes:

[ANDROID] [ENHANCEMENT] [Views] - Message

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 28, 2018
@dulmandakhdulmandakh changed the title Remove support FragmentActivity and related widgetsRemove support of FragmentActivity and related widgetsJun 28, 2018
@react-native-botreact-native-bot added ✅Test Plan Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Jun 28, 2018
@hramos

Copy link
Copy Markdown
Contributor

cc @mdvacca

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

Please review and merge @mdvacca. It removes FragmentActivity support which was required to support Fragments on Android below 3.0.

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

Please review and merge @hramos@mdvacca. Once merged I would like to work in DialogModule to fix app crashes. Thank you

@janicduplessisjanicduplessis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice this actually cleans up a lot of code!

Could you also check if it is possible to get rid of some react_native_dep("third-party/android/support/v4:lib-support-v4") in the BUCK files of the modules that don't need it anymore?

@@ -209,7 +199,7 @@ private Context getContext() {
if (mActivity != null) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be return Assertions.assertNotNull(mActivity) so we still crash if it is null.

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

@janicduplessis removed lib-support-v4 dependencies, and added assertion as you suggested. Thank you

@facebook-github-botfacebook-github-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@dulmandakh

dulmandakh commented Jul 24, 2018 via email

Copy link
Copy Markdown
ContributorAuthor

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

@hramos this is still not imported (merged). Could you please check if there is any issue, so I can help to resolve if any.

@LinusU

Copy link
Copy Markdown
Contributor

screen shot 2018-08-01 at 17 22 38

Haven't researched this too much, but it seems like the built-in FragmentManager is being deprecated in favor of the one from the support library.

Seems like the direction Android is going towards is to keep many things just in the support library (ref: https://stackoverflow.com/a/39747063/148072), so maybe we should go back to using that?

@hramos

Copy link
Copy Markdown
Contributor

Nothing wrong with the PR, it just needs to be approved internally before it can land.

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

incorporated suggestions from @kmagiera in #19741 to make transition from ReactFragmentActivity to ReactActivity easy.

@facebook-github-botfacebook-github-bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@janicduplessis

Copy link
Copy Markdown
Contributor

Yea it looks like we might have gotten this wrong, @LinusU is right and FragmentManager is being deprecated https://developer.android.com/reference/android/app/FragmentManager. Not sure if we could remove the android.app.Activity code paths instead. This would probably be a breaking change though for brownfield apps, in case they don't use AppCompatActivity.

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

@janicduplessis@LinusU IMHO, even if FragmentManager is deprecated it's not removed so we can still use it. Therefore, removing unnecessary code, used to support older versions RN don't support, will make changes or refactoring easy. Please correct me if wrong.

@LinusU

Copy link
Copy Markdown
Contributor

Therefore, removing unnecessary code, used to support older versions RN don't support, will make changes or refactoring easy.

The thing is that the concepts of using a FragmentManager and using Fragments is not going away and is still a recommended way to build your applications. It's just that the code path that's being actively developed is being moved from core to the support library.

Therefore I think it would better to remove the code that uses the built-in FragmentManager and switch all the usage over to the support library version. That is the path that Google wants developers to go forward, which is why they are deprecating the FragmentManager.

As @janicduplessis said though, that might be a breaking change, I'm not too familiar at this point to tell...

Anyhow, if we remove all this code, I think we'll just have to reintroduce it soon as the built-in classes are deprecated in the latest version of Android, and will probably be going away soon...

@LinusU

Copy link
Copy Markdown
Contributor

Looking over the diff again, I think that most changes are really good, just that we should use the support library versions instead of the core ones.

e.g.

Making ReactActivity extends AppCompatActivity is great, and AppCompatActivity is a direct extension of android.support.v4.app.FragmentActivity.

Removing the body of ReactFragmentActivity is great since it, by extension of the last point, will be a subclass of FragmentActivty.

I do not think that we should make these changes though:

  • getSupportFragmentManager() -> getFragmentManager()
  • android.support.v4.app.DialogFragment -> android.app.DialogFragment
  • android.support.v4.app.FragmentManager -> android.app.FragmentManager
  • ...

Since this will be going towards deprecated functions.

@LinusU

Copy link
Copy Markdown
Contributor

Also, one last thing, sorry for the wall of text 🙈

used to support older versions RN don't support

The majority of the development has happened inside of the support libraries version of the fragments. So it will contain bugfixes and features not present on earlier versions of Android. So it's not really binary, is it supported or not, but rather it enhances it on all versions of Android.

e.g. here you can see the Android team not accepting a pull request to fix a bug, since they want people to use the support library instead: android/android-ktx#161 (comment)

I hope that this has shed some light on this ☺️

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

@LinusU thanks for suggestions, I got the idea.

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

@hramos maybe, should I split this into smaller, less disruptive PRs?

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

@janicduplessis@LinusU I have no experience working with brownfield apps, so I have this question. Is it true that brownfield apps must extend either ReactActivity or ReactFragmentActivity to use React Native?

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

@hramos could you please try to import #20602. I'll try to split this PR into smaller, less disruptive PRs in the future.

@dulmandakh

Copy link
Copy Markdown
ContributorAuthor

After some research and though, it seems that #20602 would be more appropriate to be merged first. Then clean do some cleanups.

@dulmandakh
dulmandakh deleted the remove-fragment-activity branch February 22, 2019 14:24
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.Platform: AndroidAndroid applications.Type: EnhancementA new feature or enhancement of an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@dulmandakh@hramos@LinusU@janicduplessis@facebook-github-bot@react-native-bot