-
-
Notifications
You must be signed in to change notification settings - Fork 613
chore: add reference StackContainer implementation
#3518
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
Conversation
It does not make sense to not specify children for `StackHost`.
The original implementation is hard for me to follow. Moreover it has been based upon old operation model based on `maxLifecycleState`. New operation model introduced in RFC-0591 requires changes in implementation of stack operations, therefore I took this opportunity and reimplemented all of it. The implementation is not completed yet (nor was the previous one). It introduces single `StackContainer` component that should be used to render the new stack. While implementing I noticed a couple of problems introduced by me in RFC-0591. First - there is no easy way to get the navigation state in the JS layer. This is especially troublesome when implementing `pop` operation, as it is not clear what screen to dismiss. The exact problem lies in fact that in current model we allow for rendering array of `Stack.Screen`s with some in `detached` mode. When already rendered `Screen` changes its mode to `attached`, it becomes a top screen, but it's index in the state array does not change. Therefore, we can not really tell which screen is on top. `Pop` operation is one problem, but another is simply dumping current navigation state e.g. for telemetry purposes / saving app state before restart or something. I'm sure there are many use cases for reading current navigation state.
It logs component renders, mount & unmount. We can generalize / move it common code later. It seems useful.
`react-native-screens/private` This hook can be reused easily for different components while debugging. Seems useful enough to add it to private package.
This is rather a PoC than proper support. There are a couple of issues with current implementation.
Before this change, the information has been passed from `RNSStackScreenComponentView` to `StackController` via `StackScreenController`. This is not compliant with RFC-0753. The controllers should not be aware of activity mode changes. The information should be passed directly to `StackHost` and there request to container should be dispatched. This is exactly what this commit does.
This is debug-oriented utility.
screen Previously I checked only whether there is screen at all. This could lead to corrupted state in obvious way, where all screens were detached!
This is essential functionality that I have to add manually each time I debug something. I think since this is still higihly experimental impelmentation it is fine to have some logging there, especially that it is frequently needed to exam the stack state etc.
Please note that preloaded route is pushed as last element of the `stackState`. This can potentially cause problems with element inspector as from ShadowTree perspective the newly rendered screen is the one on the very top (drawn last), despite the fact that it won't be attached to native container. This is not a new problem. Old stack implementation suffers from the very same one. In the future we might consider disabling gestures on the 'detached' screens (pointer events). This might help to mitigate this issue.
102c8b2 to
d5db948
Compare
It is not used anywhere in computation.
kkafar
left a comment
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.
Few implementation notes and directions for future work on this PR.
| routeCopy.activityMode = 'attached'; | ||
| newState.push(routeCopy); |
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.
Please note that the route is pushed to the end of the state list here, to prevent issues with stack state restoration & element inspector.
| - (BOOL)emitOnDismiss | ||
| { | ||
| if (_reactEventEmitter != nullptr) { | ||
| _reactEventEmitter->onDismiss({}); | ||
| return YES; | ||
| } else { | ||
| RCTLogWarn(@"[RNScreens] Skipped OnDismissed event emission due to nullish emitter"); | ||
| return NO; | ||
| } | ||
| } | ||
|
|
||
| - (BOOL)emitOnNativeDismiss | ||
| { | ||
| if (_reactEventEmitter != nullptr) { | ||
| _reactEventEmitter->onNativeDismiss({}); | ||
| return YES; | ||
| } else { | ||
| RCTLogWarn(@"[RNScreens] Skipped onNativeDismiss event emission due to nullish emitter"); | ||
| return NO; | ||
| } | ||
| } |
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.
We could consider emitting only a single type of event but with boolean in payload. I think it'll be a better approach.
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.
+1
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.
I'd say two function is very slightly more readable (onDismiss could be made even more descriptive, i.e. onCompleteJSDismiss() )
and if those two diverge in the future, we wouldn't need to refactor
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.
Please see: c840b04
| public override func didMove(toParent parent: UIViewController?) { | ||
| print("ScreenCtrl [\(self.screen.tag)] didMoveToParent \(String(describing: parent))") | ||
| super.didMove(toParent: parent) | ||
|
|
||
| if parent == nil { | ||
| if self.screen.activityMode == .detached { | ||
| reactEventEmitter.emitOnDismiss() | ||
| } else { | ||
| reactEventEmitter.emitOnNativeDismiss() | ||
| } | ||
| } | ||
| } |
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.
This is absolutely not a correct implementation.
We need a proper way to distinguish a native dismiss from JS initiated one.
This is just PoC code, rough-and-ready for demonstrative purposes only. We can land it, cause it does its purpose at the moment, but we'll have to improve it 100%.
| ref?: React.RefObject< | ||
| (React.Component<NativeProps> & ReactNativeElement) | null |
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.
I made this type like that just to make things work (and it still does not work correctly). It seemed that there is something incompatible in types between RefObject to React.Component and HostComponentType (type of any native component). Dunno, needs to be investigated.
| import React from 'react'; | ||
| import { findNodeHandle, ReactNativeElement } from 'react-native'; | ||
|
|
||
| export type NativeComponentGenericRef = React.Component & ReactNativeElement; | ||
|
|
||
| export function useRenderDebugInfo<RefType extends React.Component>( | ||
| componentName: string, | ||
| ) { | ||
| const componentRef = React.useRef<RefType>(null); | ||
| const componentNodeHandle = React.useRef<number>(-1); | ||
|
|
||
| const logMessageEvent = React.useEffectEvent((message: string) => { | ||
| logMessage(componentName, componentNodeHandle.current, message); | ||
| }); | ||
|
|
||
| React.useEffect(() => { | ||
| if (componentRef.current != null) { | ||
| componentNodeHandle.current = findNodeHandle(componentRef.current) ?? -1; | ||
|
|
||
| if (componentNodeHandle.current === -1) { | ||
| logMessageEvent('failed to find node handle'); | ||
| } | ||
| } | ||
|
|
||
| logMessageEvent('mounted'); | ||
|
|
||
| return () => { | ||
| logMessageEvent('unmounted'); | ||
| }; | ||
| }, []); | ||
|
|
||
| logMessage(componentName, componentNodeHandle.current, 'rendered'); | ||
|
|
||
| return componentRef; | ||
| } | ||
|
|
||
| function logMessage( | ||
| componentName: string, | ||
| nodeHandle: number, | ||
| message: string, | ||
| ) { | ||
| console.log(`${componentName} [${nodeHandle}] ${message}`); | ||
| } |
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.
Tremendously useful thing for logging & debugging I found myself to write multiple times over and over. This time I decided to clean it a bit & add it, so that we can reuse it.
This clearly needs a doc comment explaining what is a purpose of this hook.
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.
StackContainer implementation
Previous implementation fails in scenario: A -> Aa -> AaB -> and push A again. This is because the search end on first A, which is already rendered and attached, falling through to creation of new route. Now, instead we only for detached screens that wait to be rendered. This is not perfect, because I can imagine use cases where we want detached screen, let's say A-1 to stay detached and push another, let's say A-2, e.g. in case A-1 is used for picture-in-picture. But this is concern of downstream implementation / we can figure it out later. Currently such route should not be preloaded.
kligarski
left a comment
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.
Looks good, I left some suggestions.
Looks like we still have a problem with JS/native state synchronization. As discussed internally, this is expected for now until we handle it on the native side in another PR but I guess it's worth it to document it here.
Simulator.Screen.Recording.-.iPhone.17.Pro.-.2026-01-13.at.10.40.25.mov
|
|
||
| export type StackRouteConfig = { | ||
| name: string; | ||
| Component: () => React.ReactNode; |
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.
- Should prop name
Componentbe capitalized? Or should it becomponent? - I wonder what type should be used here. For example, in
BottomTabsContainerwe useComponentType. I'm not sure what the right answer is here.
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.
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.
Ad 2.
I guess ComponentType is more appropriate here. It allows for class component to be passed, which I guess it not that big of advantage, but rather nice thing is that it correctly types Suspending components that potentially return a Promise<React.ReactNode>.
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.
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.
Okay. We should probably match that convention in BottomTabsContainer then. I created a ticket to do so: https://github.com/software-mansion/react-native-screens-labs/issues/776.
| - (BOOL)emitOnDismiss | ||
| { | ||
| if (_reactEventEmitter != nullptr) { | ||
| _reactEventEmitter->onDismiss({}); | ||
| return YES; | ||
| } else { | ||
| RCTLogWarn(@"[RNScreens] Skipped OnDismissed event emission due to nullish emitter"); | ||
| return NO; | ||
| } | ||
| } | ||
|
|
||
| - (BOOL)emitOnNativeDismiss | ||
| { | ||
| if (_reactEventEmitter != nullptr) { | ||
| _reactEventEmitter->onNativeDismiss({}); | ||
| return YES; | ||
| } else { | ||
| RCTLogWarn(@"[RNScreens] Skipped onNativeDismiss event emission due to nullish emitter"); | ||
| return NO; | ||
| } | ||
| } |
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.
+1
apps/src/shared/gamma/containers/stack/StackContainer.types.tsx
Outdated
Show resolved
Hide resolved
| - (BOOL)emitOnDismiss | ||
| { | ||
| if (_reactEventEmitter != nullptr) { | ||
| _reactEventEmitter->onDismiss({}); | ||
| return YES; | ||
| } else { | ||
| RCTLogWarn(@"[RNScreens] Skipped OnDismissed event emission due to nullish emitter"); | ||
| return NO; | ||
| } | ||
| } | ||
|
|
||
| - (BOOL)emitOnNativeDismiss | ||
| { | ||
| if (_reactEventEmitter != nullptr) { | ||
| _reactEventEmitter->onNativeDismiss({}); | ||
| return YES; | ||
| } else { | ||
| RCTLogWarn(@"[RNScreens] Skipped onNativeDismiss event emission due to nullish emitter"); | ||
| return NO; | ||
| } | ||
| } |
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.
I'd say two function is very slightly more readable (onDismiss could be made even more descriptive, i.e. onCompleteJSDismiss() )
and if those two diverge in the future, we wouldn't need to refactor
These two are replaced by single `onDismiss` event with a boolean in payload, that allows to still distinguish between these two. I've decided to leave `onNativeDismiss` callback on `StackScreen` React component, because it is handy to use in the `StackContainer` implementation.
This is just a safe guard to catch possibly annoying to debug error. This commit also formats the code. I can not get my editor to save without formatting ^_^
Before it has been done in useEffect, which is fine, but this seems cleaner. This changes behaviour a bit, because earlier when after initial render for some reason the number of routes in state was decreased to 0, the useEffect would ensure that there is at least a single route present. Now this isn't the case. The stack will end up empty. This is ok. If the stack is emptied, then something went wrong anyway & it is better to observe obviously invalid state of empty stack than to artificially obfuscate the problem.
There is no need to make these types "private".
kligarski
left a comment
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.
Looks good!
| const preloadAction = React.useCallback((routeKey: string) => { | ||
| dispatch({ type: 'preload', routeName: routeKey, ctx: actionContext }); |
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.
why in this specific case we're taking routeKey and then we're passing it to routeName, I'm just considering whether there's a chance to make dispatch call consistent with other actions without changing param name
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.
You're right here. This is a mistake. The callback's param should be named routeName and not routeKey.
| [dispatch, actionContext], | ||
| ); | ||
|
|
||
| const preloadAction = React.useCallback((routeKey: string) => { |
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.
shouldn't we add PreloadActionMethod type here?
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.
Thanks, added here: 609a140
| } | ||
|
|
||
| const newState = [...state]; | ||
| // NOTE: This modifes existing state, possibly impacting calculations done before new state is updated. |
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.
| // NOTE: This modifes existing state, possibly impacting calculations done before new state is updated. | |
| // NOTE: This modifies existing state, possibly impacting calculations done before new state is updated. |
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.

Description
This PR introduces JS implementation of
StackContainercomponent. It is not library component, but rather a container implementation that is a example of a possible downstream implementation.It is required for us to have a referential container implementation to test on.
This is not a complete implementation. It is rather a starting point, providing implementation of basic operations following RFC-0591.
Closes https://github.com/software-mansion/react-native-screens-labs/issues/771
Changes
Rename
maxLifecycleStatetoactivityModeFollowing naming from RFC-0591.
This commit also does necessary cleanup of no longer used types such as
StackScreenLifecycleState.RNSConversion-Stack is introduced with a
convertfunction template,which can be possibly later reused for other components as well.
Make type of
childrenprop ofStackHostnot key-optionalIt does not make sense to not specify children for
StackHost.Reimplement StackContainer
The original implementation is hard for me to follow. Moreover it has
been based upon old operation model based on
maxLifecycleState.New operation model introduced in RFC-0591 requires changes in
implementation of stack operations, therefore I took this opportunity
and reimplemented all of it.
The implementation is not completed yet (nor was the previous one).
It introduces single
StackContainercomponent that should be usedto render the new stack.
While implementing I noticed a couple of problems introduced by
me in RFC-0591. First - there is no easy way to get the navigation state
in the JS layer. This is especially troublesome when implementing
popoperation, as it is not clear what screen to dismiss.
The exact problem lies in fact that in current model we allow for
rendering array of
Stack.Screens with some indetachedmode.When already rendered
Screenchanges its mode toattached,it becomes a top screen, but it's index in the state array does not
change. Therefore, we can not really tell which screen is on top.
Popoperation is one problem, but another is simply dumping currentnavigation state e.g. for telemetry purposes / saving app state
before restart or something. I'm sure there are many use cases for
reading current navigation state.
Add
useRenderDebugInfoutility hookIt logs component renders, mount & unmount.
We can generalize / move it common code later.
It seems useful.
Fix action name & move useRenderDebugInfo hook to
react-native-screens/privateThis hook can be reused easily for different components while debugging.
Seems useful enough to add it to private package.
Add basic support for dismiss in JS example code
Add basic support for dismiss in component code
This is rather a PoC than proper support. There are a couple of issues
with current implementation.
Pass info on
activityModechange from StackScreen to StackHostBefore this change, the information has been passed from
RNSStackScreenComponentViewtoStackControllerviaStackScreenController. This is not compliant with RFC-0753.The controllers should not be aware of activity mode changes. The
information should be passed directly to
StackHostand there requestto container should be dispatched. This is exactly what this commit
does.
Cleanup in component code
Add navigationStateReducerWithLogging that logs actions & state
This is debug-oriented utility.
Prevent pop action from executing when there is at most one attached screen
Previously I checked only whether there is screen at all. This could
lead to corrupted state in obvious way, where all screens were detached!
Fix import in component code