facebook
feat: add listener for mount operations on Android
Summary:
This PR introduces kind of the same API that is available on iOS for subscribing to view mutations (see https://github.com/facebook/react-native/blob/67384cf5a07662b5030c20cf666b0f10b402bf23/packages/react-native/React/Fabric/Mounting/RCTMountingTransactionObserving.h) but for Android. The concept is needed there for e.g. for native-stack, with the same use case as on iOS, where we want to know that the views will be removed to start transition on them, and we need to know it before they are removed (and it is done in a bottom-up manner on new arch).
The current implementation uses UIManagerModule as the place where the listeners live, since it is possible to get to this module both from this code and from library's code. All the mutations are first added to an array, then the listeners are fired, and the changes are applied afterwards.
Changelog:
[ANDROID] [ADDED] - Listener for view mutations
Test Plan:
Run the RNTester app and see that all mutations are still mounted correctly. Add subscriber and see that you can check the mutations and act based on them in other components before they are mounted.
cc @cortinico if you could check the PR if the current direction is correct, or maybe cc someone that could help with merging it?
| Platform | Engine | Arch | Size (bytes) | Diff |
|---|---|---|---|---|
| android | hermes | arm64-v8a | 17,163,267 | +8 |
| android | hermes | armeabi-v7a | n/a | -- |
| android | hermes | x86 | n/a | -- |
| android | hermes | x86_64 | n/a | -- |
| android | jsc | arm64-v8a | 20,542,635 | +2 |
| android | jsc | armeabi-v7a | n/a | -- |
| android | jsc | x86 | n/a | -- |
| android | jsc | x86_64 | n/a | -- |
Base commit: c5258b5b2a667878f6eaccde8f8924801f43b0dd Branch: main
cc. @cortinico @arushikesarwani94 could one of you lead the landing of this? 🙏
Did anyone have a chance to look at it? It would be perfect if it could land before 0.74 is cut.
I created an internal task in our backlog. We are starting a periodical review of those pending tasks on Monday afternoon and Wednesday afternoon, so expect some answer by Tuesday, probably.
I'm sorry that it is taking so long. :(
@cortinico do you want me to rebase or does this comment does it automatically?
@cortinico do you want me to rebase or does this comment does it automatically?
@WoLewicki if you could rebase manually that would be really helpful. We're discussing internally how to move forward with this change. We'll get back to you once we know what are the next steps (rebasing is needed anyway)
Thanks for the work and for rebasing, I will take a look at it today or tomorrow.
Two things that stand up:
- we shouldn't use UIManagerModule from fabric code
- This change is in the critical path of React Native rendering, potentially affecting rendering perf during initial render of screens, @WoLewicki what's your opinion on perf implications of the PR?
we shouldn't use UIManagerModule from fabric code
Do you suggest some other approach? I'm not sure how could it be done differently.
This change is in the critical path of React Native rendering, potentially affecting rendering perf during initial render of screens, @WoLewicki what's your opinion on perf implications of the PR?
I am aware that it is critical and I haven't noticed any immediate performance drops. I'm making small, short lived objects that keep the information for listeners and then are used for dispatching the mutations. As long as it is not introducing performance decrease, it should be fine.
I am aware that it is critical and I haven't noticed any immediate performance drops. I'm making small, short lived objects that keep the information for listeners and then are used for dispatching the mutations. As long as it is not introducing performance decrease, it should be fine.
The initial implementation also relied on small short lived objects, but we've refactored the code this way because we found regressions on low end devices. I'm curious, how did you measure and confirmed there is no performance drop? We could try again, but we will need to refactor the code, add a feature flag and create an a/b testing to verify lack of performance regressions.
we shouldn't use UIManagerModule from fabric code Do you suggest some other approach? I'm not sure how could it be done differently.
One approach is adding a method in UIManager and implement it inside FabricUIManager. Although, I wonder if instead we should expose a new method inside UIManagerListener.java, and then call it from FabricUIManager.didMountItems, what do you think?
Just to provide a quick update here. We are discussing internally about adding or not this new API, if it is decided to proceed with this new API, then we will probably lead the development to move a bit faster and reduce back and forth in github reviews.
@WoLewicki it would be really helpful if you can provide concrete examples of usages of this API. for example current usages of RCTMountingTransactionObserving where you are making usage of the MountingTransaction objects. or how would you use MountingTransaction objects in Android
Thank you!
cc @cortinico
I'm curious, how did you measure and confirmed there is no performance drop?
Tbh, I did not test it extensively, just in the example app for react-native-screens. I wanted to rely on your testing to see if the performance is good enough still 😅
it would be really helpful if you can provide concrete examples of usages of this API. for example current usages of RCTMountingTransactionObserving where you are making usage of the MountingTransaction objects. or how would you use MountingTransaction objects in Android
As I mentioned in the description, it would be basically the same as on iOS in react-native-screens: https://github.com/software-mansion/react-native-screens/blob/58da3e2cb884fecd4d6ae9b51a45e9d596621fc6/ios/RNSScreenStack.mm#L1147 where we need to know if the screen from navigator will be destroyed in next transaction. Do you want me to find the exact code that would be used on Android with it?
Do you want me to find the exact code that would be used on Android with it?
We are mostly interested to understand what information you need from the mountingTranstacion/mountItem, just the type and component name as you do in iOS?
Not sure if some other libs would like to have more information, but for react-native-screens the same information as on iOS is enough now.
@WoLewicki thanks again for opening the discussion on exposing mount operations in Android and for implementing this PR
We've discussed internally about this PR and we have several concerns to move forward with it:
- This API is exposing internal implementation details of the Renderer of React Native, adding this new API in Android might block us to extend or refactor React Native renderer in the future.
- The PR is adding complexity on the execution of MountItems which can negatively affect performance on the critical path of React Native Renderer (which is already super perf sensitive in low end devices)
- The proposed use cases can we achieved by using other APIs, for example if you want to know if a view is being destroyed/deleted for the "RNSScreenStack" ViewManager, you could override the removeViewAt / removeView methods in "RNSScreenStack" ViewManager instead e.g. see: https://github.com/facebook/react-native/blob/7d47781046c53177bdae607736a3f599628c8704/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewGroupManager.java#L78-L84
- We've revisited mountingTransactionWillMount iOS API and we noticed that this API is overexposing MountingTransaction and we might revisit this in the near future. We have the goal of aligning APIs between Android and iOS, but in this case we might want to move away from exposing MountingTransaction in iOS instead of adding this API in Android.
I'm closing the PR because we decided that we won't add this API in React Native Android and we recommend overriding viewManager.removeViewAt() method instead.
CC @cortinico , @javache , @cipolleschi , @sammy-SC
The proposed use cases can we achieved by using other APIs, for example if you want to know if a view is being destroyed/deleted for the "RNSScreenStack" ViewManager, you could override the removeViewAt / removeView methods in "RNSScreenStack" ViewManager instead e.g. see:
yeah, we already use this mechanism on old arch: https://github.com/software-mansion/react-native-screens/blob/3b35894f1f83bc4c9daa518d68f3bcb0e832129a/android/src/main/java/com/swmansion/rnscreens/ScreenStackViewManager.kt#L33. The problem is the same as on iOS though. Because of the removals being dispatched bottom-up now, when we get to removeViewAt in the ScreenStack, all of the children are already unmounted and we can do nothing about it. Or am I missing something @mdvacca ?
Hey! I took another approach for getting the list of mutations that is unified between iOS and Android: https://github.com/software-mansion/react-native-screens/pull/2134. It makes subscribing to RCTMountingTransactionObserving unnecessary, which seems like the way to go! We use the mechanism that was made for LayoutAnimations, namely setMountingOverrideDelegate.
Could you elaborate if it is a proper solution? The only problem with it for now is that there can be only one delegate right now and react-native-reanimated also wants to use it for its LayoutAnimations. Should I make a PR making it an array of delegates instead of a single one? cc @mdvacca @javache @cortinico @cipolleschi @sammy-SC. Sorry for pinging you all but I am not sure who is the owner of that code.