feat: make view recycling optional on iOS

[WoLewicki]

Summary

This PR resolves the potential problem of misconfiguration of components after being recycled. Some of them have custom, sometimes native (e.g. connected to VCs) logic that messes up with the concept of recycling.

Changelog

Added shouldBeRecycled method to RCTComponentViewProtocol, a check for it in _enqueueComponentViewWithComponentHandle:(ComponentHandle)componentHandle componentViewDescriptor:(RCTComponentViewDescriptor)componentViewDescriptor method, and a default implementation in UIView+ComponentViewProtocol returning YES in order not to change the default behavior.

[iOS] [Added] - Add shouldBeRecycled method on iOS.

Test Plan

Override this method in your custom componentView and see that the component is not recycled.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,842,749	+3
android	hermes	armeabi-v7a	8,152,151	+4
android	hermes	x86	9,348,439	+4
android	hermes	x86_64	9,191,273	+4
android	jsc	arm64-v8a	9,454,435	+4
android	jsc	armeabi-v7a	8,635,756	+2
android	jsc	x86	9,537,431	+5
android	jsc	x86_64	9,780,793	+4

Base commit: f6c417c7ae221b4b54f954008c29558290c7744e Branch: main

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 0062b10b56985c4556011fbbb8d43f0a038d359e Branch: main

[pull-bot]

PR build artifact for 46a905b78eb3cffbd72d3256e09900298eee1f44 is ready. To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

[facebook-github-bot]

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

[sammy-SC]

Hello @WoLewicki,

It is possible to re-create native resource inside prepareForRecycle to work around any recycling issues. One could re-create UIViewController inside prepareForRecycle. Would that work for you? This avoids adding an exception to the logic but allows component to opt out of recycling.

[janicduplessis]

@sammy-SC If I remember correctly the issue was that a view cannot be added as the main view (VC.view property) of multiple view controllers. Once it has been added to one it cannot be added again, with no way that I found to clear that, which is where recycling is problematic.

[WoLewicki]

Hey @sammy-SC, in case of UIViewController, used by react-native-screens, iirc there were some problems with recreating view controllers and attaching screens to it since it messed with the native screen transitions. I cannot precisely recall the exact problem, but we tried that approach and it created some confusing native VC states. I believe it was due to the fact that native screen transition in react-native-screens is triggered by the change of React children, which also triggers prepareForRecycle method, so removing VC in that moment is not a good idea.

[grahammendick]

Hey, I’m the author of the Navigation router. It took me more than 6 months to migrate my library to the new architecture. I can remember spending a couple of those weeks just trying to get view recycling working with UIViewController-backed views (like scenes and tab bars).

I finally found a pattern that works. I don’t clear the UIViewController in prepareForRecycle. Instead I set a flag that indicates it should be cleared. And then when the view is used again, in updateProps or mountChildComponentView, I do that actual clearing. @WoLewicki can you give this a go and see if it works for you, please?

Recycling views was a big headache for contentInsetAdjustmentBehavior set to automatic, too. It took me another solid week to get prepareForRecycle working with large titles

[WoLewicki]

Hey @grahammendick, thanks for adding more context to it. I currently do not work on react-native-screens, so it would better to cc @kkafar I believe. Nevertheless, I think that merging this PR and removing all those hacky solutions is the way to go. wdyt of it?

[grahammendick]

I support this PR because view recycling is a massive pain for integrating with the native api on iOS. I'm still not 100% sure the pattern I settled on works in all scenarios.

But I think this PR should also override the shouldBeRecycled on the RCTScrollViewComponentView and return false if contentInsetAdjustmentBehavior is automatic; and back out the associated hack

[WoLewicki]

But I think this PR should also override the shouldBeRecycled on the RCTScrollViewComponentView and return false if contentInsetAdjustmentBehavior is automatic;

I didn't want to introduce any changes to the behavior of the components in this PR so it can be safely merged without much testing. This change can be probably made in some following PR if it is a proper solution.

[grahammendick]

@sammy-SC can you explain why you wouldn't want this escape hatch, please? What are the downsides from the React Native perspective?

[sammy-SC]

@sammy-SC can you explain why you wouldn't want this escape hatch, please? What are the downsides from the React Native perspective?

It is not that I do not want it. If it makes it easier to implement components, I'm all up for it.

I'm just being cautious to and trying to explore other options we have before merging this PR. This API is used by 3rd party components and it is a contract React Native has with components. Therefore, we need to think twice before making any change as it is difficult to undo in the future. I have a couple ideas for optimisations for the new architecture but they depend on recycling working well.

One escape hatch recycling has is to just wipe out state of view component inside prepareForRecycle. But as @WoLewicki mentioned, it did not work for him. I just would like to learn more about the use case he had and why clearing all state in prepareForRecycle did not work for him.

[WoLewicki]

I've just checked our example app on Fabric, and I can still see one scenario where our logic does not work, being calling navigation.replace() method. Due to view recycling, the same view that has been just unmounted with the previous VC, is now attached with the same VC (since we do not destroy VCs on recycling, because it would mess with the native transitions). The native platform code does not detect the change in attached VCs then so it does not perform any screen transition, leading to wrong application state. I don't remember any other issues that were not solved yet, but this one is pretty big.

[grahammendick]

That sounds like a 'bug' with react navigation because the component shouldn't unmount if it's still used by the transition. The Navigation router waits until the transition is complete before unmounting the component. I'm still supportive of the PR

[WoLewicki]

We made native-stack subscribe to the unmount on purpose to make the JS code as simple as possible 🤷‍♂️

[facebook-github-bot]

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

[WoLewicki]

@sammy-SC is this PR going to be added to the core? Should I do something about it ?

[sammy-SC]

@WoLewicki I'm on board with having a way to opt out of view recycling. You have made some compelling arguments why this is useful and I think it would make writing native components easier.

I don't think it should live on ComponentView itself though. This gives impression that each component instance can choose whether it is recyclable or not. But this isn't the case. It is always type of component that is recycle, not individual instances. Therefore method shouldBeRecycled should be probably a static method and its value stored in RCTComponentViewClassDescriptor.

[WoLewicki]

@sammy-SC I moved it to RCTComponentViewClassDescriptor as you suggested, but left the check for selector only, so the value returned by it is not taken into account. Is it a problem? If someone wants to subscribe to it, I think it should mean that he wants to disable this behavior.

[WoLewicki]

Ok I pushed a better (hopefully) version that checks it correctly. Could you check it again @sammy-SC ?

[cipolleschi]

@WoLewicki can I ask you to rebase on top of main? I think that this diff is pointing to a very old commit of RN.

[WoLewicki]

@cipolleschi should be the newest main now.

[facebook-github-bot]

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

[facebook-github-bot]

@cipolleschi merged this pull request in facebook/react-native@613a5a75977d84333df7cbd5701e01a7ab5a3385.