Skip to content

Commit 939baeb

Browse files
WoLewickitboba
authored and
ja1ns
committed
feat: dont recycle RNScreenView (software-mansion#2069)
## Description PR disabling view recycling from `RNSScreenView` component. It also enables us to fix long lasting issue with view recycling. This feature was added in RN 0.74 and will only work then. Fixes software-mansion#1628. ## Changes - stop recycling `RNSScreenView` - Remove logic connected to problems with view recycling - Change the logic of setting push view controllers on new architecture, when transition is ongoing - Add if check for cases, when user navigates to more than one screen at the same time ## Test code and steps to reproduce See that modals and `replace` action work correctly. You can also use `Test2069.tsx` from FabricTestExample / TestsExample to test the behavior of replacing screens conditionally. --------- Co-authored-by: tboba <[email protected]>
1 parent 5d069fd commit 939baeb

File tree

7 files changed

+234
-71
lines changed

7 files changed

+234
-71
lines changed

‎FabricTestExample/App.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ import TestScreenAnimation from './src/TestScreenAnimation';
9494
import Test1981 from './src/Test1981';
9595
import Test2008 from './src/Test2008';
9696
import Test2028 from './src/Test2028';
97+
import Test2069 from './src/Test2069';
9798

9899
enableFreeze(true);
99100

‎FabricTestExample/src/Test2069.tsx‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
import { NavigationContainer } from '@react-navigation/native';
2+
import {
3+
createNativeStackNavigator,
4+
NativeStackNavigationProp,
5+
} from '@react-navigation/native-stack';
6+
import React, { useState } from 'react';
7+
import { StyleSheet, Text, TouchableOpacity, View } from 'react-native';
8+
9+
type StackParamList = {
10+
Home: undefined;
11+
Home1: undefined;
12+
Home2: undefined;
13+
Home3: undefined;
14+
};
15+
16+
interface MainScreenProps {
17+
navigation: NativeStackNavigationProp<StackParamList>;
18+
}
19+
20+
const Home = ({ navigation }: MainScreenProps) => (
21+
<View style={styles.view}>
22+
<Text
23+
onPress={() => {
24+
navigation.navigate('Home1');
25+
navigation.navigate('Home2');
26+
}}>
27+
This is the initial View
28+
</Text>
29+
</View>
30+
);
31+
32+
const Home1 = ({ navigation }: MainScreenProps) => (
33+
<View style={styles.view}>
34+
<Text onPress={() => navigation.goBack()}>This is View 1</Text>
35+
</View>
36+
);
37+
38+
const Home2 = ({ navigation }: MainScreenProps) => (
39+
<View style={styles.view}>
40+
<Text onPress={() => navigation.goBack()}>This is View 2</Text>
41+
</View>
42+
);
43+
44+
const Home3 = ({ navigation }: MainScreenProps) => (
45+
<View style={styles.view}>
46+
{/* goBack shouldn't work here. */}
47+
<Text onPress={() => navigation.goBack()}>This is View 3</Text>
48+
</View>
49+
);
50+
51+
const Stack = createNativeStackNavigator();
52+
53+
const Test2069 = () => {
54+
const [hasChangedState, setHasChangedState] = useState(0);
55+
56+
return (
57+
<NavigationContainer>
58+
<Stack.Navigator>
59+
{hasChangedState % 3 === 0 ? (
60+
<>
61+
<Stack.Screen name="Home" component={Home} />
62+
<Stack.Screen name="Home1" component={Home1} />
63+
<Stack.Screen name="Home2" component={Home2} />
64+
</>
65+
) : hasChangedState % 3 === 1 ? (
66+
<>
67+
<Stack.Screen name="Home2" component={Home2} />
68+
</>
69+
) : (
70+
<>
71+
<Stack.Screen name="Home3" component={Home3} />
72+
</>
73+
)}
74+
</Stack.Navigator>
75+
<TouchableOpacity
76+
style={styles.button}
77+
onPress={() => setHasChangedState(old => old + 1)}>
78+
<Text>Change state</Text>
79+
</TouchableOpacity>
80+
</NavigationContainer>
81+
);
82+
};
83+
84+
const styles = StyleSheet.create({
85+
button: {
86+
justifyContent: 'center',
87+
alignItems: 'center',
88+
height: 100,
89+
},
90+
view: {
91+
alignItems: 'center',
92+
backgroundColor: '#b7c4bb',
93+
flex: 1,
94+
justifyContent: 'center',
95+
padding: 12,
96+
},
97+
});
98+
99+
export default Test2069;

‎TestsExample/App.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ import Test1844 from './src/Test1844';
9595
import Test1864 from './src/Test1864';
9696
import Test1981 from './src/Test1981';
9797
import Test2008 from './src/Test2008';
98+
import Test2069 from './src/Test2069';
9899

99100
enableFreeze(true);
100101

‎TestsExample/src/Test2069.tsx‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
import { NavigationContainer } from '@react-navigation/native';
2+
import {
3+
createNativeStackNavigator,
4+
NativeStackNavigationProp,
5+
} from '@react-navigation/native-stack';
6+
import React, { useState } from 'react';
7+
import { StyleSheet, Text, TouchableOpacity, View } from 'react-native';
8+
9+
type StackParamList = {
10+
Home: undefined;
11+
Home1: undefined;
12+
Home2: undefined;
13+
Home3: undefined;
14+
};
15+
16+
interface MainScreenProps {
17+
navigation: NativeStackNavigationProp<StackParamList>;
18+
}
19+
20+
const Home = ({ navigation }: MainScreenProps) => (
21+
<View style={styles.view}>
22+
<Text
23+
onPress={() => {
24+
navigation.navigate('Home1');
25+
navigation.navigate('Home2');
26+
}}>
27+
This is the initial View
28+
</Text>
29+
</View>
30+
);
31+
32+
const Home1 = ({ navigation }: MainScreenProps) => (
33+
<View style={styles.view}>
34+
<Text onPress={() => navigation.goBack()}>This is View 1</Text>
35+
</View>
36+
);
37+
38+
const Home2 = ({ navigation }: MainScreenProps) => (
39+
<View style={styles.view}>
40+
<Text onPress={() => navigation.goBack()}>This is View 2</Text>
41+
</View>
42+
);
43+
44+
const Home3 = ({ navigation }: MainScreenProps) => (
45+
<View style={styles.view}>
46+
{/* goBack shouldn't work here. */}
47+
<Text onPress={() => navigation.goBack()}>This is View 3</Text>
48+
</View>
49+
);
50+
51+
const Stack = createNativeStackNavigator();
52+
53+
const Test2069 = () => {
54+
const [hasChangedState, setHasChangedState] = useState(0);
55+
56+
return (
57+
<NavigationContainer>
58+
<Stack.Navigator>
59+
{hasChangedState % 3 === 0 ? (
60+
<>
61+
<Stack.Screen name="Home" component={Home} />
62+
<Stack.Screen name="Home1" component={Home1} />
63+
<Stack.Screen name="Home2" component={Home2} />
64+
</>
65+
) : hasChangedState % 3 === 1 ? (
66+
<>
67+
<Stack.Screen name="Home2" component={Home2} />
68+
</>
69+
) : (
70+
<>
71+
<Stack.Screen name="Home3" component={Home3} />
72+
</>
73+
)}
74+
</Stack.Navigator>
75+
<TouchableOpacity
76+
style={styles.button}
77+
onPress={() => setHasChangedState(old => old + 1)}>
78+
<Text>Change state</Text>
79+
</TouchableOpacity>
80+
</NavigationContainer>
81+
);
82+
};
83+
84+
const styles = StyleSheet.create({
85+
button: {
86+
justifyContent: 'center',
87+
alignItems: 'center',
88+
height: 100,
89+
},
90+
view: {
91+
alignItems: 'center',
92+
backgroundColor: '#b7c4bb',
93+
flex: 1,
94+
justifyContent: 'center',
95+
padding: 12,
96+
},
97+
});
98+
99+
export default Test2069;

‎ios/RNSScreen.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ namespace react = facebook::react;
4141
- (RNSScreenView *)screenView;
4242
#ifdef RCT_NEW_ARCH_ENABLED
4343
- (void)setViewToSnapshot:(UIView *)snapshot;
44-
- (void)resetViewToScreen;
4544
- (CGFloat)calculateHeaderHeightIsModal:(BOOL)isModal;
4645
#endif
4746

‎ios/RNSScreen.mm‎

Lines changed: 13 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -171,12 +171,10 @@ - (void)setStackPresentation:(RNSScreenStackPresentation)stackPresentation
171171
_controller.presentationController.delegate = self;
172172
} else if (_stackPresentation != RNSScreenStackPresentationPush) {
173173
#ifdef RCT_NEW_ARCH_ENABLED
174-
// TODO: on Fabric, same controllers can be used as modals and then recycled and used a push which would result in
175-
// this error. It would be good to check if it doesn't leak in such case.
176174
#else
177175
RCTLogError(
178176
@"Screen presentation updated from modal to push, this may likely result in a screen object leakage. If you need to change presentation style create a new screen object instead");
179-
#endif
177+
#endif // RCT_NEW_ARCH_ENABLED
180178
}
181179
_stackPresentation = stackPresentation;
182180
}
@@ -298,7 +296,6 @@ - (void)notifyDismissedWithCount:(int)dismissCount
298296
{
299297
#ifdef RCT_NEW_ARCH_ENABLED
300298
// If screen is already unmounted then there will be no event emitter
301-
// it will be cleaned in prepareForRecycle
302299
if (_eventEmitter != nullptr) {
303300
std::dynamic_pointer_cast<const react::RNSScreenEventEmitter>(_eventEmitter)
304301
->onDismissed(react::RNSScreenEventEmitter::OnDismissed{.dismissCount = dismissCount});
@@ -320,7 +317,6 @@ - (void)notifyDismissCancelledWithDismissCount:(int)dismissCount
320317
{
321318
#ifdef RCT_NEW_ARCH_ENABLED
322319
// If screen is already unmounted then there will be no event emitter
323-
// it will be cleaned in prepareForRecycle
324320
if (_eventEmitter != nullptr) {
325321
std::dynamic_pointer_cast<const react::RNSScreenEventEmitter>(_eventEmitter)
326322
->onNativeDismissCancelled(
@@ -337,7 +333,6 @@ - (void)notifyWillAppear
337333
{
338334
#ifdef RCT_NEW_ARCH_ENABLED
339335
// If screen is already unmounted then there will be no event emitter
340-
// it will be cleaned in prepareForRecycle
341336
if (_eventEmitter != nullptr) {
342337
std::dynamic_pointer_cast<const react::RNSScreenEventEmitter>(_eventEmitter)
343338
->onWillAppear(react::RNSScreenEventEmitter::OnWillAppear{});
@@ -360,7 +355,6 @@ - (void)notifyWillDisappear
360355
}
361356
#ifdef RCT_NEW_ARCH_ENABLED
362357
// If screen is already unmounted then there will be no event emitter
363-
// it will be cleaned in prepareForRecycle
364358
if (_eventEmitter != nullptr) {
365359
std::dynamic_pointer_cast<const react::RNSScreenEventEmitter>(_eventEmitter)
366360
->onWillDisappear(react::RNSScreenEventEmitter::OnWillDisappear{});
@@ -376,7 +370,6 @@ - (void)notifyAppear
376370
{
377371
#ifdef RCT_NEW_ARCH_ENABLED
378372
// If screen is already unmounted then there will be no event emitter
379-
// it will be cleaned in prepareForRecycle
380373
if (_eventEmitter != nullptr) {
381374
std::dynamic_pointer_cast<const react::RNSScreenEventEmitter>(_eventEmitter)
382375
->onAppear(react::RNSScreenEventEmitter::OnAppear{});
@@ -396,7 +389,6 @@ - (void)notifyDisappear
396389
{
397390
#ifdef RCT_NEW_ARCH_ENABLED
398391
// If screen is already unmounted then there will be no event emitter
399-
// it will be cleaned in prepareForRecycle
400392
if (_eventEmitter != nullptr) {
401393
std::dynamic_pointer_cast<const react::RNSScreenEventEmitter>(_eventEmitter)
402394
->onDisappear(react::RNSScreenEventEmitter::OnDisappear{});
@@ -676,6 +668,11 @@ - (BOOL)hasHeaderConfig
676668
return react::concreteComponentDescriptorProvider<react::RNSScreenComponentDescriptor>();
677669
}
678670

671+
+ (BOOL)shouldBeRecycled
672+
{
673+
return NO;
674+
}
675+
679676
- (void)mountChildComponentView:(UIView<RCTComponentViewProtocol> *)childComponentView index:(NSInteger)index
680677
{
681678
if ([childComponentView isKindOfClass:[RNSScreenStackHeaderConfig class]]) {
@@ -697,27 +694,6 @@ - (void)unmountChildComponentView:(UIView<RCTComponentViewProtocol> *)childCompo
697694

698695
#pragma mark - RCTComponentViewProtocol
699696

700-
- (void)prepareForRecycle
701-
{
702-
[super prepareForRecycle];
703-
// TODO: Make sure that there is no edge case when this should be uncommented
704-
// _controller=nil;
705-
_dismissed = NO;
706-
_state.reset();
707-
_touchHandler = nil;
708-
709-
// We set this prop to default value here to workaround view-recycling.
710-
// Let's assume the view has had _stackPresentation == <some modal stack presentation> set
711-
// before below line was executed. Then, when instantiated again (with the same modal presentation)
712-
// updateProps:oldProps: method would be called and setter for stack presentation would not be called.
713-
// This is crucial as in that setter we register `self.controller` as a delegate
714-
// (UIAdaptivePresentationControllerDelegate) to presentation controller and this leads to buggy modal behaviour as we
715-
// rely on UIAdaptivePresentationControllerDelegate callbacks. Restoring the default value and then comparing against
716-
// it in updateProps:oldProps: allows for setter to be called, however if there was some additional logic to execute
717-
// when stackPresentation is set to "push" the setter would not be triggered.
718-
_stackPresentation = RNSScreenStackPresentationPush;
719-
}
720-
721697
- (void)updateProps:(react::Props::Shared const &)props oldProps:(react::Props::Shared const &)oldProps
722698
{
723699
const auto &oldScreenProps = *std::static_pointer_cast<const react::RNSScreenProps>(_props);
@@ -781,12 +757,9 @@ - (void)updateProps:(react::Props::Shared const &)props oldProps:(react::Props::
781757
}
782758
#endif // !TARGET_OS_TV
783759

784-
// Notice that we compare against _stackPresentation, not oldScreenProps.stackPresentation.
785-
// See comment in prepareForRecycle method for explanation.
786-
RNSScreenStackPresentation newStackPresentation =
787-
[RNSConvert RNSScreenStackPresentationFromCppEquivalent:newScreenProps.stackPresentation];
788-
if (newStackPresentation != _stackPresentation) {
789-
[self setStackPresentation:newStackPresentation];
760+
if (newScreenProps.stackPresentation != oldScreenProps.stackPresentation) {
761+
[self
762+
setStackPresentation:[RNSConvert RNSScreenStackPresentationFromCppEquivalent:newScreenProps.stackPresentation]];
790763
}
791764

792765
if (newScreenProps.stackAnimation != oldScreenProps.stackAnimation) {
@@ -993,9 +966,6 @@ - (void)viewDidAppear:(BOOL)animated
993966
- (void)viewDidDisappear:(BOOL)animated
994967
{
995968
[super viewDidDisappear:animated];
996-
#ifdef RCT_NEW_ARCH_ENABLED
997-
[self resetViewToScreen];
998-
#endif
999969
if (self.parentViewController == nil && self.presentingViewController == nil) {
1000970
if (self.screenView.preventNativeDismiss) {
1001971
// if we want to prevent the native dismiss, we do not send dismissal event,
@@ -1411,25 +1381,10 @@ - (void)hideHeaderIfNecessary
14111381

14121382
- (void)setViewToSnapshot:(UIView *)snapshot
14131383
{
1414-
// modals of native stack seem not to support
1415-
// changing their view by just setting the view
1416-
if (_initialView.stackPresentation != RNSScreenStackPresentationPush) {
1417-
UIView *superView = self.view.superview;
1418-
[self.view removeFromSuperview];
1419-
self.view = snapshot;
1420-
[superView addSubview:self.view];
1421-
} else {
1422-
[self.view removeFromSuperview];
1423-
self.view = snapshot;
1424-
}
1425-
}
1426-
1427-
- (void)resetViewToScreen
1428-
{
1429-
if (self.view != _initialView) {
1430-
[self.view removeFromSuperview];
1431-
self.view = _initialView;
1432-
}
1384+
UIView *superView = self.view.superview;
1385+
[self.view removeFromSuperview];
1386+
self.view = snapshot;
1387+
[superView addSubview:self.view];
14331388
}
14341389

14351390
#else

0 commit comments

Comments
 (0)