Skip to content

Commit

Permalink
feat: dont recycle RNScreenView (#2069)
Browse files Browse the repository at this point in the history
## 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 #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 <tymoteusz.boba@gmail.com>
  • Loading branch information
WoLewicki and tboba committed Mar 20, 2024
1 parent 09db75e commit 3b35894
Show file tree
Hide file tree
Showing 7 changed files with 234 additions and 71 deletions.
1 change: 1 addition & 0 deletions FabricTestExample/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ import TestScreenAnimation from './src/TestScreenAnimation';
import Test1981 from './src/Test1981';
import Test2008 from './src/Test2008';
import Test2028 from './src/Test2028';
import Test2069 from './src/Test2069';

enableFreeze(true);

Expand Down
99 changes: 99 additions & 0 deletions FabricTestExample/src/Test2069.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import { NavigationContainer } from '@react-navigation/native';
import {
createNativeStackNavigator,
NativeStackNavigationProp,
} from '@react-navigation/native-stack';
import React, { useState } from 'react';
import { StyleSheet, Text, TouchableOpacity, View } from 'react-native';

type StackParamList = {
Home: undefined;
Home1: undefined;
Home2: undefined;
Home3: undefined;
};

interface MainScreenProps {
navigation: NativeStackNavigationProp<StackParamList>;
}

const Home = ({ navigation }: MainScreenProps) => (
<View style={styles.view}>
<Text
onPress={() => {
navigation.navigate('Home1');
navigation.navigate('Home2');
}}>
This is the initial View
</Text>
</View>
);

const Home1 = ({ navigation }: MainScreenProps) => (
<View style={styles.view}>
<Text onPress={() => navigation.goBack()}>This is View 1</Text>
</View>
);

const Home2 = ({ navigation }: MainScreenProps) => (
<View style={styles.view}>
<Text onPress={() => navigation.goBack()}>This is View 2</Text>
</View>
);

const Home3 = ({ navigation }: MainScreenProps) => (
<View style={styles.view}>
{/* goBack shouldn't work here. */}
<Text onPress={() => navigation.goBack()}>This is View 3</Text>
</View>
);

const Stack = createNativeStackNavigator();

const Test2069 = () => {
const [hasChangedState, setHasChangedState] = useState(0);

return (
<NavigationContainer>
<Stack.Navigator>
{hasChangedState % 3 === 0 ? (
<>
<Stack.Screen name="Home" component={Home} />
<Stack.Screen name="Home1" component={Home1} />
<Stack.Screen name="Home2" component={Home2} />
</>
) : hasChangedState % 3 === 1 ? (
<>
<Stack.Screen name="Home2" component={Home2} />
</>
) : (
<>
<Stack.Screen name="Home3" component={Home3} />
</>
)}
</Stack.Navigator>
<TouchableOpacity
style={styles.button}
onPress={() => setHasChangedState(old => old + 1)}>
<Text>Change state</Text>
</TouchableOpacity>
</NavigationContainer>
);
};

const styles = StyleSheet.create({
button: {
justifyContent: 'center',
alignItems: 'center',
height: 100,
},
view: {
alignItems: 'center',
backgroundColor: '#b7c4bb',
flex: 1,
justifyContent: 'center',
padding: 12,
},
});

export default Test2069;
1 change: 1 addition & 0 deletions TestsExample/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ import Test1844 from './src/Test1844';
import Test1864 from './src/Test1864';
import Test1981 from './src/Test1981';
import Test2008 from './src/Test2008';
import Test2069 from './src/Test2069';

enableFreeze(true);

Expand Down
99 changes: 99 additions & 0 deletions TestsExample/src/Test2069.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import { NavigationContainer } from '@react-navigation/native';
import {
createNativeStackNavigator,
NativeStackNavigationProp,
} from '@react-navigation/native-stack';
import React, { useState } from 'react';
import { StyleSheet, Text, TouchableOpacity, View } from 'react-native';

type StackParamList = {
Home: undefined;
Home1: undefined;
Home2: undefined;
Home3: undefined;
};

interface MainScreenProps {
navigation: NativeStackNavigationProp<StackParamList>;
}

const Home = ({ navigation }: MainScreenProps) => (
<View style={styles.view}>
<Text
onPress={() => {
navigation.navigate('Home1');
navigation.navigate('Home2');
}}>
This is the initial View
</Text>
</View>
);

const Home1 = ({ navigation }: MainScreenProps) => (
<View style={styles.view}>
<Text onPress={() => navigation.goBack()}>This is View 1</Text>
</View>
);

const Home2 = ({ navigation }: MainScreenProps) => (
<View style={styles.view}>
<Text onPress={() => navigation.goBack()}>This is View 2</Text>
</View>
);

const Home3 = ({ navigation }: MainScreenProps) => (
<View style={styles.view}>
{/* goBack shouldn't work here. */}
<Text onPress={() => navigation.goBack()}>This is View 3</Text>
</View>
);

const Stack = createNativeStackNavigator();

const Test2069 = () => {
const [hasChangedState, setHasChangedState] = useState(0);

return (
<NavigationContainer>
<Stack.Navigator>
{hasChangedState % 3 === 0 ? (
<>
<Stack.Screen name="Home" component={Home} />
<Stack.Screen name="Home1" component={Home1} />
<Stack.Screen name="Home2" component={Home2} />
</>
) : hasChangedState % 3 === 1 ? (
<>
<Stack.Screen name="Home2" component={Home2} />
</>
) : (
<>
<Stack.Screen name="Home3" component={Home3} />
</>
)}
</Stack.Navigator>
<TouchableOpacity
style={styles.button}
onPress={() => setHasChangedState(old => old + 1)}>
<Text>Change state</Text>
</TouchableOpacity>
</NavigationContainer>
);
};

const styles = StyleSheet.create({
button: {
justifyContent: 'center',
alignItems: 'center',
height: 100,
},
view: {
alignItems: 'center',
backgroundColor: '#b7c4bb',
flex: 1,
justifyContent: 'center',
padding: 12,
},
});

export default Test2069;
1 change: 0 additions & 1 deletion ios/RNSScreen.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ namespace react = facebook::react;
- (RNSScreenView *)screenView;
#ifdef RCT_NEW_ARCH_ENABLED
- (void)setViewToSnapshot:(UIView *)snapshot;
- (void)resetViewToScreen;
- (CGFloat)calculateHeaderHeightIsModal:(BOOL)isModal;
#endif

Expand Down
71 changes: 13 additions & 58 deletions ios/RNSScreen.mm
Original file line number Diff line number Diff line change
Expand Up @@ -171,12 +171,10 @@ - (void)setStackPresentation:(RNSScreenStackPresentation)stackPresentation
_controller.presentationController.delegate = self;
} else if (_stackPresentation != RNSScreenStackPresentationPush) {
#ifdef RCT_NEW_ARCH_ENABLED
// TODO: on Fabric, same controllers can be used as modals and then recycled and used a push which would result in
// this error. It would be good to check if it doesn't leak in such case.
#else
RCTLogError(
@"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");
#endif
#endif // RCT_NEW_ARCH_ENABLED
}
_stackPresentation = stackPresentation;
}
Expand Down Expand Up @@ -298,7 +296,6 @@ - (void)notifyDismissedWithCount:(int)dismissCount
{
#ifdef RCT_NEW_ARCH_ENABLED
// If screen is already unmounted then there will be no event emitter
// it will be cleaned in prepareForRecycle
if (_eventEmitter != nullptr) {
std::dynamic_pointer_cast<const react::RNSScreenEventEmitter>(_eventEmitter)
->onDismissed(react::RNSScreenEventEmitter::OnDismissed{.dismissCount = dismissCount});
Expand All @@ -320,7 +317,6 @@ - (void)notifyDismissCancelledWithDismissCount:(int)dismissCount
{
#ifdef RCT_NEW_ARCH_ENABLED
// If screen is already unmounted then there will be no event emitter
// it will be cleaned in prepareForRecycle
if (_eventEmitter != nullptr) {
std::dynamic_pointer_cast<const react::RNSScreenEventEmitter>(_eventEmitter)
->onNativeDismissCancelled(
Expand All @@ -337,7 +333,6 @@ - (void)notifyWillAppear
{
#ifdef RCT_NEW_ARCH_ENABLED
// If screen is already unmounted then there will be no event emitter
// it will be cleaned in prepareForRecycle
if (_eventEmitter != nullptr) {
std::dynamic_pointer_cast<const react::RNSScreenEventEmitter>(_eventEmitter)
->onWillAppear(react::RNSScreenEventEmitter::OnWillAppear{});
Expand All @@ -360,7 +355,6 @@ - (void)notifyWillDisappear
}
#ifdef RCT_NEW_ARCH_ENABLED
// If screen is already unmounted then there will be no event emitter
// it will be cleaned in prepareForRecycle
if (_eventEmitter != nullptr) {
std::dynamic_pointer_cast<const react::RNSScreenEventEmitter>(_eventEmitter)
->onWillDisappear(react::RNSScreenEventEmitter::OnWillDisappear{});
Expand All @@ -376,7 +370,6 @@ - (void)notifyAppear
{
#ifdef RCT_NEW_ARCH_ENABLED
// If screen is already unmounted then there will be no event emitter
// it will be cleaned in prepareForRecycle
if (_eventEmitter != nullptr) {
std::dynamic_pointer_cast<const react::RNSScreenEventEmitter>(_eventEmitter)
->onAppear(react::RNSScreenEventEmitter::OnAppear{});
Expand All @@ -396,7 +389,6 @@ - (void)notifyDisappear
{
#ifdef RCT_NEW_ARCH_ENABLED
// If screen is already unmounted then there will be no event emitter
// it will be cleaned in prepareForRecycle
if (_eventEmitter != nullptr) {
std::dynamic_pointer_cast<const react::RNSScreenEventEmitter>(_eventEmitter)
->onDisappear(react::RNSScreenEventEmitter::OnDisappear{});
Expand Down Expand Up @@ -676,6 +668,11 @@ - (BOOL)hasHeaderConfig
return react::concreteComponentDescriptorProvider<react::RNSScreenComponentDescriptor>();
}

+ (BOOL)shouldBeRecycled
{
return NO;
}

- (void)mountChildComponentView:(UIView<RCTComponentViewProtocol> *)childComponentView index:(NSInteger)index
{
if ([childComponentView isKindOfClass:[RNSScreenStackHeaderConfig class]]) {
Expand All @@ -697,27 +694,6 @@ - (void)unmountChildComponentView:(UIView<RCTComponentViewProtocol> *)childCompo

#pragma mark - RCTComponentViewProtocol

- (void)prepareForRecycle
{
[super prepareForRecycle];
// TODO: Make sure that there is no edge case when this should be uncommented
// _controller=nil;
_dismissed = NO;
_state.reset();
_touchHandler = nil;

// We set this prop to default value here to workaround view-recycling.
// Let's assume the view has had _stackPresentation == <some modal stack presentation> set
// before below line was executed. Then, when instantiated again (with the same modal presentation)
// updateProps:oldProps: method would be called and setter for stack presentation would not be called.
// This is crucial as in that setter we register `self.controller` as a delegate
// (UIAdaptivePresentationControllerDelegate) to presentation controller and this leads to buggy modal behaviour as we
// rely on UIAdaptivePresentationControllerDelegate callbacks. Restoring the default value and then comparing against
// it in updateProps:oldProps: allows for setter to be called, however if there was some additional logic to execute
// when stackPresentation is set to "push" the setter would not be triggered.
_stackPresentation = RNSScreenStackPresentationPush;
}

- (void)updateProps:(react::Props::Shared const &)props oldProps:(react::Props::Shared const &)oldProps
{
const auto &oldScreenProps = *std::static_pointer_cast<const react::RNSScreenProps>(_props);
Expand Down Expand Up @@ -781,12 +757,9 @@ - (void)updateProps:(react::Props::Shared const &)props oldProps:(react::Props::
}
#endif // !TARGET_OS_TV

// Notice that we compare against _stackPresentation, not oldScreenProps.stackPresentation.
// See comment in prepareForRecycle method for explanation.
RNSScreenStackPresentation newStackPresentation =
[RNSConvert RNSScreenStackPresentationFromCppEquivalent:newScreenProps.stackPresentation];
if (newStackPresentation != _stackPresentation) {
[self setStackPresentation:newStackPresentation];
if (newScreenProps.stackPresentation != oldScreenProps.stackPresentation) {
[self
setStackPresentation:[RNSConvert RNSScreenStackPresentationFromCppEquivalent:newScreenProps.stackPresentation]];
}

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

- (void)setViewToSnapshot:(UIView *)snapshot
{
// modals of native stack seem not to support
// changing their view by just setting the view
if (_initialView.stackPresentation != RNSScreenStackPresentationPush) {
UIView *superView = self.view.superview;
[self.view removeFromSuperview];
self.view = snapshot;
[superView addSubview:self.view];
} else {
[self.view removeFromSuperview];
self.view = snapshot;
}
}

- (void)resetViewToScreen
{
if (self.view != _initialView) {
[self.view removeFromSuperview];
self.view = _initialView;
}
UIView *superView = self.view.superview;
[self.view removeFromSuperview];
self.view = snapshot;
[superView addSubview:self.view];
}

#else
Expand Down

0 comments on commit 3b35894

Please sign in to comment.