Migrate ReactClippingViewGroup to Kotlin#49413
Conversation
facebook-github-bot
added
the
CLA Signed
facebook-github-bot
added
the
Shared with Meta
| @get:Deprecated("Use removeClippedSubviews property instead", ReplaceWith("removeClippedSubviews")) | ||
| @Suppress("INAPPLICABLE_JVM_NAME") | ||
| @get:JvmName("getRemoveClippedSubviews") |
There was a problem hiding this comment.
This feels wrong. Why do we need all of this? Specifically the ReplaceWith won't work as you're replacing with the same string
There was a problem hiding this comment.
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
| override var removeClippedSubviews: Boolean = false | ||
| set(value) { |
There was a problem hiding this comment.
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]
There was a problem hiding this comment.
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: BooleanBut 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?
There was a problem hiding this comment.
So we need to look only at Kotlin usages here
I believe the latter as just forks, so we can consider this actually [INTERNAL]
There was a problem hiding this comment.
Ohh, understood. Seems good then, thank you for double checking!
|
@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@cortinico merged this pull request in 7f1edbd. |
|
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? |
|
@mateoguzmana @cortinico This change is being reverted since crash was found due to this change.
|
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
| } | ||
| } | ||
|
|
||
| super.setRemoveClippedSubviews(removeClippedSubviews) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
5 participants
Summary:
Migrate com.facebook.react.uimanager.ReactClippingViewGroup to Kotlin
Changelog:
[INTERNAL] - Migrate com.facebook.react.uimanager.ReactClippingViewGroup to Kotlin
Test Plan: