feat: add support testID in Drawer #11009
Closed
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
Problem:
When using nested navigators, unmounts cause race cleanup races.
Imagine following hierarchy (from tree root downwards, parent to children):
TabNavigator (1) [renders useNavigationBuilder]
SceneView (from TabNavigator)
StackNavigators (N) [each renders useNavigationBuilder]
SceneView (from StackNavigator)
Now lets test following flow:
1. Mount above navigators with given navigation params (e.g. navigation for unauthenticated users)
2. Unmount all navigators (e.g. during login process)
3. Mount above navigation with different navigation params than in 1) (e.g. navigation for authenticated users)
What you'll observe, there will be old navigation params preserved in 3) coming from 1).
Source of problem:
BaseNavigationContainer holds global navigation state, exposes API to modify it via NavigationStateContext. When useNavigationBuilder unmounts, it attempts to clear navigation state. (see cleanup effect in useNavigationBuilder.tsx).
(I) First clear occurs in TabNavigator's effect, which successfully clears BaseNavigationContainer's state (sets it to undefined).
(II) Second clear comes from StackNavigator unmount. It's useNavigationBuilder cleanup effect also calls NavigationStateContext.setState(undefined).
But this time - we meet SceneView as closest NavigationStateContext.Provider. SceneView attempts to merge state change with current navigation state, which is reasonable. But current navigation state should be already undefined... It is, but:
```
[useNavigationBuilder.tsx]
const getState = React.useCallback((): State => {
const currentState = getCurrentState();
return isStateInitialized(currentState)
? (currentState as State)
: (initializedStateRef.current as State);
}, [getCurrentState, isStateInitialized]);
```
"undefined" state is treated is non-initialized state, and freshly computed state (initializedStateRef.current) is returned instead.
SceneView does merge this old state with `undefined` value, and passes to BaseNavigationContainer. Now we have some legacy global state, despite all navigators being unmounted.
After mounting navigators again (3), we can observe old params being restored. These params might come e.g. from old `initialParams` prop (from 1)).
Solution:
Do not propagate `setState` upwards in `useNavigationBuilder` after state cleanup. This way we'll omit such races.
- @react-navigation/bottom-tabs@6.0.0-next.21 - @react-navigation/core@6.0.0-next.16 - @react-navigation/devtools@6.0.0-next.17 - @react-navigation/drawer@6.0.0-next.20 - @react-navigation/elements@1.0.0-next.20 - flipper-plugin-react-navigation@1.3.1 - @react-navigation/material-bottom-tabs@6.0.0-next.17 - @react-navigation/material-top-tabs@6.0.0-next.17 - @react-navigation/native-stack@6.0.0-next.10 - @react-navigation/native@6.0.0-next.16 - @react-navigation/stack@6.0.0-next.28
This fixes an issue where the new actions could bring back the params even after it was reset
- @react-navigation/bottom-tabs@6.0.0-next.22 - @react-navigation/core@6.0.0-next.17 - @react-navigation/devtools@6.0.0-next.18 - @react-navigation/drawer@6.0.0-next.21 - @react-navigation/elements@1.0.0-next.21 - flipper-plugin-react-navigation@1.3.2 - @react-navigation/material-bottom-tabs@6.0.0-next.18 - @react-navigation/material-top-tabs@6.0.0-next.18 - @react-navigation/native-stack@6.0.0-next.11 - @react-navigation/native@6.0.0-next.17 - @react-navigation/stack@6.0.0-next.29
- @react-navigation/devtools@6.0.0-next.19
This is useful for libraries like `expo-auth-session` which also use links for authentication.
Usage:
```js
const linking = {
prefixes: ['myapp://'],
filter: (url) => !url.includes('+expo-auth-session'),
};
```
- @react-navigation/bottom-tabs@6.0.0 - @react-navigation/core@6.0.0 - @react-navigation/devtools@6.0.0 - @react-navigation/drawer@6.0.0 - @react-navigation/elements@1.0.0 - flipper-plugin-react-navigation@1.3.3 - @react-navigation/material-bottom-tabs@6.0.0 - @react-navigation/material-top-tabs@6.0.0 - @react-navigation/native-stack@6.0.0 - @react-navigation/native@6.0.0 - @react-navigation/routers@6.0.0 - @react-navigation/stack@6.0.0
This reverts commit 54b215b.
…date` (#10871) Changes introduced in #10767 created a regression reported in #10856 which made it impossible to close a modal with a gesture. As we want to change pointer-events on Card in componentDidUpdate previous use of setNativeProps in this context made a lot of sense. Since setNativeProps is going to be removed in the new architecture we had to migrate it to the component state as stated in the migration guide. This PR adds an additional `closing !== prevProps.closing` check on top of #10767 within componentDidUpdate in Card.tsx.
- @react-navigation/stack@6.3.2
- @react-navigation/native-stack@6.9.1
- @react-navigation/material-top-tabs@6.3.0
- @react-navigation/stack@6.3.3
…10972) Starting and building the web version of our Example app on Node v18 results in a build error described in webpack/webpack#14532 because of a breaking change in hashing algorithms used in newer Node.js versions (>=17). This issue got fixed in Webpack 5 but Webpack authors decided not to backport this fix to v4. Expo developer @byCedric suggests using this workaround: expo/expo-cli#4575 (comment) before Expo itself upgrades to Webpack 5. We have to pass that env variable to pretty much every command we use eg. before `build` script. Using `yarn lerna run prepack` directly still results in an error.
### Motivation This PR is a third attempt to migrate setNativeProps to state in @react-navigation/stack following the official Moving setNativeProps to state guide. After issues with the implementation of #10767 and more problems with the fix in #10871 we had to take a step back and rethink where the problem lays with these implementations. The solution was to move the state inside the CardSheet component and use useImperativeHandle to hoist a setter function up to Card to avoid triggering a rerender during Card animate function.
- flipper-plugin-react-navigation@1.3.15 - @react-navigation/stack@6.3.4
**Motivation** This PR supersedes a deprecated `Platform.isTVOS` which got removed in facebook/react-native#34071 for the currently used standard - a `Platform.isTV` check. `Platform.isTV` check exists since react-native `v0.61` that way no breaking changes are introduced to react-navigation. Ref: software-mansion/react-native-screens#1605
…nsparent is true (#10993) **Motivation** As reported in [#10845](#10845), the behavior that native software in the iOS ecosystem applies is that even transparent headers _can_ have a border in the NavigationBar/TopBar. In React Navigation, it was assumed that `headerTransparent=true` would also imply `hideShadow=true`, which would prevent this behavior (and therefore, make the behavior of this library inconsistent with native apps) This fix allows the behavior to continue on Android, while allowing the native iOS behavior to be present (also providing the option to hide it forcefully). Issue here: #10845 **Test plan** - Configure a NativeStack to have a transparent header on iOS - Observe that this does not affect the `hideShadow` property on iOS - Observe that behavior on Android is unchanged | Bug | Expected | :-------------------------:|:-------------------------:  |  The change must pass lint, typescript and tests: ✅
Since we're already using GitHub actions for a lot of things, this PR moves away from CircleCI for our checks. This will consolidate caching etc. and simplify the workflows.
|
Hey awpogodin! Thanks for opening your first pull request in this repo. If you haven't already, make sure to read our contribution guidelines. |
✅ Deploy Preview for react-navigation-example ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Hello, the PR got closed automatically due to some changes in the main branch. Would you be able to send a new PR? Thank you. |
|
Hey! This issue is closed and isn't watched by the core team. You are welcome to discuss the issue with others in this thread, but if you think this issue is still valid and needs to be tracked, please open a new issue with a repro. |
satya164
pushed a commit
that referenced
this pull request
Feb 12, 2023
satya164
pushed a commit
that referenced
this pull request
Feb 12, 2023
satya164
pushed a commit
that referenced
this pull request
Feb 17, 2023
satya164
pushed a commit
that referenced
this pull request
Feb 17, 2023
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.
Added support for TestIDs for auto-tests
#7492