-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Conversation
cc @mdvacca |
Please review and merge @mdvacca. It removes FragmentActivity support which was required to support Fragments on Android below 3.0. |
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.
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) { |
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 think this should be return Assertions.assertNotNull(mActivity)
so we still crash if it is null.
@janicduplessis removed lib-support-v4 dependencies, and added assertion as you suggested. Thank you |
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.
@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@hramos is there any issue regarding this PR?
|
@hramos this is still not imported (merged). Could you please check if there is any issue, so I can help to resolve if any. |
Haven't researched this too much, but it seems like the built-in 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? |
Nothing wrong with the PR, it just needs to be approved internally before it can land. |
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.
hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
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. |
@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. |
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... |
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 Removing the body of I do not think that we should make these changes though:
Since this will be going towards deprecated functions. |
Also, one last thing, sorry for the wall of text 🙈
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 |
@LinusU thanks for suggestions, I got the idea. |
@hramos maybe, should I split this into smaller, less disruptive PRs? |
@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? |
After some research and though, it seems that #20602 would be more appropriate to be merged first. Then clean do some cleanups. |
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