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

Remove support of FragmentActivity and related widgets #19950

Closed
wants to merge 18 commits into from

Conversation

dulmandakh
Copy link
Contributor

@dulmandakh dulmandakh commented Jun 28, 2018

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-bot facebook-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
@dulmandakh dulmandakh changed the title Remove support FragmentActivity and related widgets Remove support of FragmentActivity and related widgets Jun 28, 2018
@react-native-bot react-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
Contributor

hramos commented Jun 28, 2018

cc @mdvacca

@dulmandakh
Copy link
Contributor Author

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

@dulmandakh
Copy link
Contributor Author

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

Copy link
Contributor

@janicduplessis janicduplessis left a comment

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
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
Contributor Author

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

Copy link
Contributor

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

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
Copy link
Contributor Author

dulmandakh commented Jul 24, 2018 via email

@dulmandakh
Copy link
Contributor Author

@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
Contributor

LinusU commented Aug 1, 2018

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
Contributor

hramos commented Aug 1, 2018

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

@dulmandakh
Copy link
Contributor Author

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

Copy link
Contributor

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

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
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
Contributor Author

@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
Contributor

LinusU commented Aug 8, 2018

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
Contributor

LinusU commented Aug 8, 2018

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
Contributor

LinusU commented Aug 8, 2018

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
Contributor Author

@LinusU thanks for suggestions, I got the idea.

@dulmandakh
Copy link
Contributor Author

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

@dulmandakh
Copy link
Contributor Author

@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
Contributor Author

@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
Contributor Author

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

@dulmandakh dulmandakh closed this Aug 13, 2018
@dulmandakh dulmandakh deleted the remove-fragment-activity branch February 22, 2019 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants