Migrate `ReactClippingViewGroup` to Kotlin #49413

mateoguzmana · 2025-02-13T22:20:48Z

Summary:

Migrate com.facebook.react.uimanager.ReactClippingViewGroup to Kotlin

Changelog:

[INTERNAL] - Migrate com.facebook.react.uimanager.ReactClippingViewGroup to Kotlin

Test Plan:

yarn test-android
yarn android

cortinico · 2025-02-14T09:41:52Z

...act-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ReactClippingViewGroup.kt

+  @get:Deprecated("Use removeClippedSubviews property instead", ReplaceWith("removeClippedSubviews"))
+  @Suppress("INAPPLICABLE_JVM_NAME")
+  @get:JvmName("getRemoveClippedSubviews")


This feels wrong. Why do we need all of this? Specifically the ReplaceWith won't work as you're replacing with the same string

Ah, I see it now. I was overlooking it, added these annotations for backwards compatibility based on other examples in the codebase when trying to fix some issues post-migration for ReactHorizontalScrollContainerLegacyView in old arch but just double checked and indeed are not needed. Thanks for pointing this out

cortinico · 2025-02-14T15:35:45Z

...id/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollContainerLegacyView.kt

+  override var removeClippedSubviews: Boolean = false
+    set(value) {


This is probably an indicator of a breaking change. We should check how often ReactClippingViewGroup is implemented in OSS. If the number is relevant this will have to be marked as [BREAKING] rather than internal.

If there are no relevant usages on the other hand, we can make this class [INTERNAL]

I see some usages in OSS (ref), so it would be a breaking change indeed.

Now, my memory started getting back to why I added the backwards compatibility annotations: seems like we can prevent this from being a breaking change by adding the following (tested by reverting the changes in ReactHorizontalScrollContainerLegacyView):

@Suppress("INAPPLICABLE_JVM_NAME") @set:JvmName("setRemoveClippedSubviews") public var removeClippedSubviews: Boolean

But in the other hand, I don't see any other cases where we have done this in the codebase. Do we prefer to have it as a breaking change?

So we need to look only at Kotlin usages here

and here

I believe the latter as just forks, so we can consider this actually [INTERNAL]

Ohh, understood. Seems good then, thank you for double checking!

facebook-github-bot · 2025-02-17T17:13:08Z

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

facebook-github-bot · 2025-02-18T16:01:02Z

@cortinico merged this pull request in 7f1edbd.

react-native-bot · 2025-02-18T16:01:10Z

This pull request was successfully merged by @mateoguzmana in 7f1edbd

^{When will my fix make it into a release? | How to file a pick request?}

alanleedev · 2025-02-20T23:58:21Z

@mateoguzmana @cortinico This change is being reverted since crash was found due to this change.
I haven't try to fix it since it was blocking a release.
But this conversion leaves sandwiched Java file which seem dangerous as it has caused issues before; Kotlin (interface) -> Java (child) -> Kotlin (grandchild)

~~We should generally avoid these kind of conversions and convert the top level interface/class last.~~
[update] issue was found and fixed in PR #49607

Summary: Reverting facebook#49413 as it was causing a crash with assertion failure in ReactViewGroup.addViewWithSubviewClippingEnabled() Changelog: [INTERNAL] revert Kotlin conversion due to crash Reviewed By: sbuggay Differential Revision: D69953848

Summary: Pull Request resolved: #49586 Reverting #49413 as it was causing a crash with assertion failure in ReactViewGroup.addViewWithSubviewClippingEnabled() Changelog: [INTERNAL] revert Kotlin conversion due to crash Reviewed By: sbuggay Differential Revision: D69953848 fbshipit-source-id: b438fb928a4849f3dbad6a9d59d0f48449035fd6

alanleedev · 2025-02-21T16:21:50Z

...id/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollContainerLegacyView.kt

+      }
    }

-    super.setRemoveClippedSubviews(removeClippedSubviews)


So I think this is why this Kotlin migration introduced a problem.
Previously, setRemoveClippedSubviews() was a function but was converted to Kotlin property var removeClippedSubViews in the top-level interface.
Java implementing this interface can use setter and getter functions, so can still use the same code.
But if a child of Java class is implemented in Kotlin and tries to use the Kotlin property in the interface that information seems to be lost due to parent being a Java class.

~~We should avoid going through multiple levels of type changes like Kotlin -> Java -> Kotlin as the original type could be lost and create in unexpected results.~~

[update] see comments in PR #49607

We should avoid going through multiple levels of type changes like Kotlin -> Java -> Kotlin as the original type could be lost and create in unexpected results.

Yeah this is bad :|

Summary: Reland of #49413 which was reverted due to an internal crash. I've attempted to do a solution to keep backwards compatibility but doesn't seem to work – keeping the original solution for now, perhaps something else can be cleaned up to avoid the breakage. ## Changelog: [INTERNAL] - Migrate com.facebook.react.uimanager.ReactClippingViewGroup to Kotlin Pull Request resolved: #49607 Test Plan: ```bash yarn test-android yarn android ``` Verified that the update changes do not cause a crash. Test flow: - login to BizApp using Instagram account - on home screen scroll down to Insights section - it should show without crashing Reviewed By: arushikesarwani94 Differential Revision: D70200000 Pulled By: alanleedev fbshipit-source-id: 89bc948c1b91d9419d4b6e1885d949c4a3c20986

Summary: Migrate com.facebook.react.uimanager.ReactClippingViewGroup to Kotlin ## Changelog: [INTERNAL] - Migrate com.facebook.react.uimanager.ReactClippingViewGroup to Kotlin Pull Request resolved: facebook#49413 Test Plan: ```bash yarn test-android yarn android ``` Reviewed By: rshest Differential Revision: D69750532 Pulled By: cortinico fbshipit-source-id: 50ec87a71b3bd523e1a9518b8bd683a027a4b422

Summary: Pull Request resolved: facebook#49586 Reverting facebook#49413 as it was causing a crash with assertion failure in ReactViewGroup.addViewWithSubviewClippingEnabled() Changelog: [INTERNAL] revert Kotlin conversion due to crash Reviewed By: sbuggay Differential Revision: D69953848 fbshipit-source-id: b438fb928a4849f3dbad6a9d59d0f48449035fd6

…acebook#49607) Summary: Reland of facebook#49413 which was reverted due to an internal crash. I've attempted to do a solution to keep backwards compatibility but doesn't seem to work – keeping the original solution for now, perhaps something else can be cleaned up to avoid the breakage. ## Changelog: [INTERNAL] - Migrate com.facebook.react.uimanager.ReactClippingViewGroup to Kotlin Pull Request resolved: facebook#49607 Test Plan: ```bash yarn test-android yarn android ``` Verified that the update changes do not cause a crash. Test flow: - login to BizApp using Instagram account - on home screen scroll down to Insights section - it should show without crashing Reviewed By: arushikesarwani94 Differential Revision: D70200000 Pulled By: alanleedev fbshipit-source-id: 89bc948c1b91d9419d4b6e1885d949c4a3c20986

Kotlinify ReactClippingViewGroup

c2198a5

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 Feb 13, 2025

mateoguzmana marked this pull request as ready for review February 13, 2025 22:52

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Feb 14, 2025

cortinico requested changes Feb 14, 2025

View reviewed changes

Remove unnnecessary annotations for backwards compatibility

121ef7a

mateoguzmana requested a review from cortinico February 14, 2025 15:09

cortinico reviewed Feb 14, 2025

View reviewed changes

facebook-github-bot closed this in 7f1edbd Feb 18, 2025

facebook-github-bot added the Merged This PR has been merged. label Feb 18, 2025

alanleedev mentioned this pull request Feb 21, 2025

Back out "Migrate ReactClippingViewGroup to Kotlin" #49586

Closed

alanleedev reviewed Feb 21, 2025

View reviewed changes

mateoguzmana mentioned this pull request Feb 21, 2025

Reland: Migrate ReactClippingViewGroup to Kotlin (#49413) #49607

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Migrate `ReactClippingViewGroup` to Kotlin #49413

Migrate `ReactClippingViewGroup` to Kotlin #49413

Uh oh!

mateoguzmana commented Feb 13, 2025

Uh oh!

cortinico Feb 14, 2025

Uh oh!

mateoguzmana Feb 14, 2025

Uh oh!

cortinico Feb 14, 2025

Uh oh!

mateoguzmana Feb 14, 2025

Uh oh!

cortinico Feb 14, 2025

Uh oh!

mateoguzmana Feb 15, 2025

Uh oh!

facebook-github-bot commented Feb 17, 2025

Uh oh!

facebook-github-bot commented Feb 18, 2025

Uh oh!

react-native-bot commented Feb 18, 2025

Uh oh!

alanleedev commented Feb 20, 2025 •

edited

Loading

Uh oh!

alanleedev Feb 21, 2025 •

edited

Loading

Uh oh!

cortinico Feb 21, 2025

Uh oh!

Uh oh!

		override var removeClippedSubviews: Boolean = false
		set(value) {

Migrate ReactClippingViewGroup to Kotlin #49413

Migrate ReactClippingViewGroup to Kotlin #49413

Uh oh!

Conversation

mateoguzmana commented Feb 13, 2025

Summary:

Changelog:

Test Plan:

Uh oh!

cortinico Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

mateoguzmana Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

cortinico Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

mateoguzmana Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

cortinico Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

mateoguzmana Feb 15, 2025

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 17, 2025

Uh oh!

facebook-github-bot commented Feb 18, 2025

Uh oh!

react-native-bot commented Feb 18, 2025

Uh oh!

alanleedev commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alanleedev Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cortinico Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Migrate `ReactClippingViewGroup` to Kotlin #49413

Migrate `ReactClippingViewGroup` to Kotlin #49413

alanleedev commented Feb 20, 2025 •

edited

Loading

alanleedev Feb 21, 2025 •

edited

Loading