-
Notifications
You must be signed in to change notification settings - Fork 24k
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
feat: make view recycling optional on iOS #35378
Conversation
Base commit: f6c417c |
Base commit: 0062b10 |
PR build artifact for 46a905b is ready. |
@mdvacca has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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.
LGTM
Hello @WoLewicki, It is possible to re-create native resource inside |
@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. |
Hey @sammy-SC, in case of |
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 I finally found a pattern that works. I don’t clear the Recycling views was a big headache for |
Hey @grahammendick, thanks for adding more context to it. I currently do not work on |
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 |
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. |
@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. One escape hatch recycling has is to just wipe out state of view component inside |
I've just checked our example app on Fabric, and I can still see one scenario where our logic does not work, being calling |
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 |
We made |
@sammy-SC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@sammy-SC is this PR going to be added to the core? Should I do something about it ? |
@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 |
@sammy-SC I moved it to |
Ok I pushed a better (hopefully) version that checks it correctly. Could you check it again @sammy-SC ? |
packages/react-native/React/Fabric/Mounting/RCTComponentViewClassDescriptor.h
Outdated
Show resolved
Hide resolved
…assDescriptor.h Co-authored-by: Riccardo Cipolleschi <riccardo.cipolleschi@gmail.com>
@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. |
@cipolleschi should be the newest main now. |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@cipolleschi merged this pull request in 613a5a7. |
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. bypass-github-export-checks ## Changelog Added `shouldBeRecycled` field checking to `RCTComponentViewClassDescriptor `, a check for it in `_enqueueComponentViewWithComponentHandle:(ComponentHandle)componentHandle componentViewDescriptor:(RCTComponentViewDescriptor)componentViewDescriptor` method, and a default implementation in `RCTComponentViewDescriptor` returning `YES` in order not to change the default behavior. [iOS] [Added] - Add `shouldBeRecycled` method on `iOS`. Pull Request resolved: facebook#35378 Test Plan: Override this method in your custom `componentView` and see that the component is not recycled. Reviewed By: javache Differential Revision: D41381683 Pulled By: cipolleschi fbshipit-source-id: 10fd1e88f99b3608767c0b57fad462837924f02a
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:
[iOS][Added] - Add
shouldBeRecycled
method oniOS
. AddedshouldBeRecycled
field checking toRCTComponentViewClassDescriptor
, a check for it in_enqueueComponentViewWithComponentHandle:(ComponentHandle)componentHandle componentViewDescriptor:(RCTComponentViewDescriptor)componentViewDescriptor
method, and a default implementation inRCTComponentViewDescriptor
returningYES
in order not to change the default behavior.Test Plan
Override this method in your custom
componentView
and see that the component is not recycled.