feat: allow preload using activityState #2389
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kkafar
added a commit
to react-navigation/react-navigation
that referenced
this pull request
Oct 8, 2024
Preload feature implemented in #12083, with the difference of renaming prop `shouldBePreloaded` into `isPreloaded` and reusing `activityState` instead of new `hiddenFromStack`. Matching PR in react-native-screens repo: software-mansion/react-native-screens#2389 --------- Co-authored-by: Maksymilian Galas <maksymilian.galas@swmansion.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Kacper Kafara <kacperkafara@gmail.com>
kkafar
approved these changes
Oct 10, 2024
| @@ -165,6 +166,7 @@ export const InnerScreen = React.forwardRef<View, ScreenProps>( | |||
| function InnerScreen(props, ref) { | |||
| const innerRef = React.useRef<ViewConfig | null>(null); | |||
| React.useImperativeHandle(ref, () => innerRef.current!, []); | |||
| const prevActivityState = usePrevious(props.activityState); | |||
There was a problem hiding this comment.
I don't like running another useEffect every render, however this should be ok for now, since we want to relax this constraint in future.
This was referenced Oct 14, 2024
kkafar
added a commit
that referenced
this pull request
Oct 15, 2024
## Description We had an issue introduced in #2389 that added the preload. We temporarily have an assumption in native stack, that the `activityState` can only progress when `Screen` component is rendered (directly) inside native stack navigator. This invariant was enforced with appropriate checks on native side. It turns out that we were wrong in our way of checking whether the `Screen` belongs to native stack or not - we have forgotten about `RNSContainerNavigationController` class that is used when rendering `ScreenContainer` with `hasTwoStates: true` and which inherits from `RNSNavigationController`. ## Changes ~Added a `isNativeStackViewController` method to `RNSNavigationController` and overrode it in subclass.~ Added `RNSScreenView#isNativeStackScreen` method, which checks whether our direct react superview is a `RNSScreenStackView`. I've found checking for `_controller.navigationController` not reliable, as `navigationController` can be found arbitrarily high in the view hierarchy. ## Test code and steps to reproduce Reported by Expo. If you want to reproduce this: create a JS stack navigator with bottom tabs inside (with `hasTwoStates: true`), and then a regular JS stack inside & try to navigate to any screen. We have not caught that earlier, because we're not testing regular JS stack at all. ## Checklist - [ ] Included code example that can be used to test this change - [x] Ensured that CI passes
3 tasks
kkafar
added a commit
that referenced
this pull request
Oct 15, 2024
## Description We did not set default value of `activityState` on iOS. It was set to `0` by default `int` initializer. In `Screen` component native spec we set it to `-1`, however this had no impact, because the value setter `setActivityStateOrNil:` filtered out the `-1` value. TODO: 1. [x] Verify that change in default value do not break any other navigator! ^ Looks like this will work, because navigators such as bottom tabs always set the activity state explicitly. This is a problem, because now, after adding preload support #2389 we filter out screens with `activityState == 0` in native stack. Therefore, we now unintentionally force our API user to always set desired `activityState`. ## Changes Set `activityState` to `-1` by default on native side. ## Test code and steps to reproduce Just render screen inside native-stack w/o `activityState` set. It won't be shown. ## Checklist - [ ] Included code example that can be used to test this change - [ ] Ensured that CI passes
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR is a rewrite of #2278 using already existing
activityState-> to avoid introducing redundant prop.Connected PR to react-navigation: react-navigation/react-navigation#12189
Test code and steps to reproduce
You can play with the preload feature in
TestPreload.tsxTo test assertion of activityState progress go to
TestActivityStateProgression.tsx- keep in mind that if the Screen hasisNativeStackit will just run JS validation, remove the prop to test native checks.Checklist