DrawerStatusContext should be exported #10976
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
…gation#9688) 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.
…navigation#10919) Test plan: Tested by rendering a dummy `View` for the header: ```js options={{ header: () => ( <View style={{ backgroundColor: 'rgba(0, 0, 0, 0.5)', height: 56, }} /> ), headerTransparent: true, }} ``` Fixes react-navigation#10822 Closes react-navigation#10902
- @react-navigation/native-stack@6.9.1
- @react-navigation/material-top-tabs@6.3.0
…entDidUpdate` (react-navigation#10871)" This reverts commit b9fb2d1.
- @react-navigation/stack@6.3.3
|
Hey leonchabbey! 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. |
leonchabbey
force-pushed
the
fix/export-drawer-status-context
branch
from
298cf5f
to
5726fb9
Compare
Codecov ReportBase: 75.46% // Head: 75.45% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #10976 +/- ##
==========================================
- Coverage 75.46% 75.45% -0.01%
==========================================
Files 167 166 -1
Lines 5131 5130 -1
Branches 1987 1987
==========================================
- Hits 3872 3871 -1
Misses 1221 1221
Partials 38 38 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
…eact-navigation#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 react-navigation#10767 and more problems with the fix in react-navigation#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 (react-navigation#10993) **Motivation** As reported in [react-navigation#10845](react-navigation#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: react-navigation#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.
|
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
Dec 4, 2022
**Motivation** `DrawerStatusContext` should be exported and available to the library's users. For example, someone might want to create a custom Drawer navigator using `useNavigationBuilder` and reuse this context. (Previous PR: #10976)
satya164
pushed a commit
that referenced
this pull request
Dec 11, 2022
**Motivation** `DrawerStatusContext` should be exported and available to the library's users. For example, someone might want to create a custom Drawer navigator using `useNavigationBuilder` and reuse this context. (Previous PR: #10976)
satya164
pushed a commit
that referenced
this pull request
Feb 17, 2023
**Motivation** `DrawerStatusContext` should be exported and available to the library's users. For example, someone might want to create a custom Drawer navigator using `useNavigationBuilder` and reuse this context. (Previous PR: #10976)
satya164
pushed a commit
that referenced
this pull request
Feb 17, 2023
**Motivation** `DrawerStatusContext` should be exported and available to the library's users. For example, someone might want to create a custom Drawer navigator using `useNavigationBuilder` and reuse this context. (Previous PR: #10976)
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.
Motivation
DrawerStatusContextshould be exported and available to the library's users.For example, someone might want to create a custom Drawer navigator using
useNavigationBuilderand reuse this context.