From c34609967afc2748a887354eb587ad57b7865a53 Mon Sep 17 00:00:00 2001 From: Satyajit Sahoo Date: Fri, 3 Nov 2023 02:34:47 +0100 Subject: [PATCH] refactor: rework how sync state is handled This simplifies the sync store for navigation state: - Use a external store to store the state instead of `useState` - so the store can be updated synchronously - Use `useSyncExternalStore` API to subscribe to the state changes - Use an effect when a child navigator needs to update state due to list of screens changing etc. The changes should make the code easier to understand, as well as should make the code compatible with concurrent mode as we're not doing anything weird with the state. --- packages/core/src/BaseNavigationContainer.tsx | 49 +++++----- packages/core/src/__tests__/index.test.tsx | 70 ++++++++++++++- .../src/useIsomorphicLayoutEffect.native.tsx | 3 + .../core/src/useIsomorphicLayoutEffect.tsx | 7 ++ packages/core/src/useNavigationBuilder.tsx | 45 ++++------ packages/core/src/useScheduleUpdate.tsx | 32 ------- packages/core/src/useSyncState.tsx | 90 +++++++------------ .../src/__tests__/ServerContainer.test.tsx | 20 +++-- 8 files changed, 155 insertions(+), 161 deletions(-) create mode 100644 packages/core/src/useIsomorphicLayoutEffect.native.tsx create mode 100644 packages/core/src/useIsomorphicLayoutEffect.tsx delete mode 100644 packages/core/src/useScheduleUpdate.tsx diff --git a/packages/core/src/BaseNavigationContainer.tsx b/packages/core/src/BaseNavigationContainer.tsx index 5cc696dbdf..50a9becb75 100644 --- a/packages/core/src/BaseNavigationContainer.tsx +++ b/packages/core/src/BaseNavigationContainer.tsx @@ -33,7 +33,6 @@ import { useEventEmitter } from './useEventEmitter'; import { useKeyedChildListeners } from './useKeyedChildListeners'; import { useNavigationIndependentTree } from './useNavigationIndependentTree'; import { useOptionsGetters } from './useOptionsGetters'; -import { ScheduleUpdateContext } from './useScheduleUpdate'; import { useSyncState } from './useSyncState'; type State = NavigationState | PartialState | undefined; @@ -103,10 +102,9 @@ export const BaseNavigationContainer = React.forwardRef( ); } - const [state, getState, setState, scheduleUpdate, flushUpdates] = - useSyncState(() => - getPartialState(initialState == null ? undefined : initialState) - ); + const [state, getState, setState] = useSyncState(() => + getPartialState(initialState == null ? undefined : initialState) + ); const isFirstMountRef = React.useRef(true); @@ -205,7 +203,7 @@ export const BaseNavigationContainer = React.forwardRef( isFocused: () => true, canGoBack, getParent: () => undefined, - getState: () => stateRef.current, + getState, getRootState, getCurrentRoute, getCurrentOptions, @@ -218,6 +216,7 @@ export const BaseNavigationContainer = React.forwardRef( getCurrentOptions, getCurrentRoute, getRootState, + getState, isReady, resetRoot, ] @@ -262,11 +261,6 @@ export const BaseNavigationContainer = React.forwardRef( [addListener, addKeyedListener, onDispatchAction, onOptionsChange] ); - const scheduleContext = React.useMemo( - () => ({ scheduleUpdate, flushUpdates }), - [scheduleUpdate, flushUpdates] - ); - const isInitialRef = React.useRef(true); const getIsInitial = React.useCallback(() => isInitialRef.current, []); @@ -294,13 +288,11 @@ export const BaseNavigationContainer = React.forwardRef( const onReadyRef = React.useRef(onReady); const onStateChangeRef = React.useRef(onStateChange); - const stateRef = React.useRef(state); React.useEffect(() => { isInitialRef.current = false; onStateChangeRef.current = onStateChange; onReadyRef.current = onReady; - stateRef.current = state; }); const onReadyCalledRef = React.useRef(false); @@ -448,26 +440,25 @@ export const BaseNavigationContainer = React.forwardRef( if (state !== previousState && stateForNextRouteNamesChange !== null) { setStateForNextRouteNamesChange(null); } + return ( - - - - - + + + + - - {children} - - - - - - + {children} + + + + + ); diff --git a/packages/core/src/__tests__/index.test.tsx b/packages/core/src/__tests__/index.test.tsx index a0fe7d1587..e247bcf058 100644 --- a/packages/core/src/__tests__/index.test.tsx +++ b/packages/core/src/__tests__/index.test.tsx @@ -430,8 +430,6 @@ it("doesn't update state if action wasn't handled", () => { }); it('cleans up state when the navigator unmounts', () => { - jest.useFakeTimers(); - const TestNavigator = (props: any) => { const { state, descriptors } = useNavigationBuilder(MockRouter, props); @@ -481,8 +479,6 @@ it('cleans up state when the navigator unmounts', () => { ); - act(() => jest.runAllTimers()); - expect(onStateChange).toHaveBeenCalledTimes(2); expect(onStateChange).toHaveBeenLastCalledWith(undefined); }); @@ -536,6 +532,72 @@ it('allows state updates by dispatching a function returning an action', () => { }); }); +it('re-initializes state once for conditional rendering', () => { + const TestNavigatorA = (props: any) => { + const { state, descriptors } = useNavigationBuilder(MockRouter, props); + + return descriptors[state.routes[state.index].key].render(); + }; + + const TestNavigatorB = (props: any) => { + const { state, descriptors } = useNavigationBuilder(MockRouter, props); + + return descriptors[state.routes[state.index].key].render(); + }; + + const onStateChange = jest.fn(); + + const navigation = createNavigationContainerRef(); + + const Test = ({ condition }: { condition: boolean }) => { + return ( + + {condition ? ( + + {() => null} + {() => null} + + ) : ( + + {() => null} + {() => null} + + )} + + ); + }; + + const root = render(); + + expect(onStateChange).toHaveBeenCalledTimes(0); + expect(navigation.getRootState()).toEqual({ + stale: false, + type: 'test', + index: 0, + key: '0', + routeNames: ['foo', 'bar'], + routes: [ + { key: 'foo', name: 'foo' }, + { key: 'bar', name: 'bar' }, + ], + }); + + root.update(); + + expect(onStateChange).toHaveBeenCalledTimes(1); + expect(onStateChange).toHaveBeenCalledWith({ + stale: false, + type: 'test', + index: 0, + key: '1', + routeNames: ['bar', 'baz'], + routes: [ + { key: 'bar', name: 'bar' }, + { key: 'baz', name: 'baz' }, + ], + }); +}); + it('updates route params with setParams', () => { const TestNavigator = (props: any) => { const { state, descriptors } = useNavigationBuilder(MockRouter, props); diff --git a/packages/core/src/useIsomorphicLayoutEffect.native.tsx b/packages/core/src/useIsomorphicLayoutEffect.native.tsx new file mode 100644 index 0000000000..b93b801c65 --- /dev/null +++ b/packages/core/src/useIsomorphicLayoutEffect.native.tsx @@ -0,0 +1,3 @@ +import { useLayoutEffect } from 'react'; + +export const useIsomorphicLayoutEffect = useLayoutEffect; diff --git a/packages/core/src/useIsomorphicLayoutEffect.tsx b/packages/core/src/useIsomorphicLayoutEffect.tsx new file mode 100644 index 0000000000..9dd3cd360c --- /dev/null +++ b/packages/core/src/useIsomorphicLayoutEffect.tsx @@ -0,0 +1,7 @@ +import { useEffect, useLayoutEffect } from 'react'; + +/** + * Use `useEffect` during SSR and `useLayoutEffect` in the browser to avoid warnings. + */ +export const useIsomorphicLayoutEffect = + typeof document !== 'undefined' ? useLayoutEffect : useEffect; diff --git a/packages/core/src/useNavigationBuilder.tsx b/packages/core/src/useNavigationBuilder.tsx index cf0c672994..627f382144 100644 --- a/packages/core/src/useNavigationBuilder.tsx +++ b/packages/core/src/useNavigationBuilder.tsx @@ -38,13 +38,13 @@ import { type ScreenConfigWithParent, useDescriptors } from './useDescriptors'; import { useEventEmitter } from './useEventEmitter'; import { useFocusedListenersChildrenAdapter } from './useFocusedListenersChildrenAdapter'; import { useFocusEvents } from './useFocusEvents'; +import { useIsomorphicLayoutEffect } from './useIsomorphicLayoutEffect'; import { useKeyedChildListeners } from './useKeyedChildListeners'; import { useNavigationHelpers } from './useNavigationHelpers'; import { useOnAction } from './useOnAction'; import { useOnGetState } from './useOnGetState'; import { useOnRouteFocus } from './useOnRouteFocus'; import { useRegisterNavigator } from './useRegisterNavigator'; -import { useScheduleUpdate } from './useScheduleUpdate'; // This is to make TypeScript compiler happy PrivateValueStore; @@ -339,12 +339,7 @@ export function useNavigationBuilder< const stateCleanedUp = React.useRef(false); - const cleanUpState = React.useCallback(() => { - setCurrentState(undefined); - stateCleanedUp.current = true; - }, [setCurrentState]); - - const setState = React.useCallback( + const setState = useLatestCallback( (state: NavigationState | PartialState | undefined) => { if (stateCleanedUp.current) { // State might have been already cleaned up due to unmount @@ -352,9 +347,9 @@ export function useNavigationBuilder< // This would lead to old data preservation on main navigator unmount return; } + setCurrentState(state); - }, - [setCurrentState] + } ); const [initializedState, isFirstStateInitialization] = React.useMemo(() => { @@ -547,7 +542,7 @@ export function useNavigationBuilder< const shouldUpdate = state !== nextState; - useScheduleUpdate(() => { + useIsomorphicLayoutEffect(() => { if (shouldUpdate) { // If the state needs to be updated, we'll schedule an update setState(nextState); @@ -571,31 +566,21 @@ export function useNavigationBuilder< return () => { // We need to clean up state for this navigator on unmount - // We do it in a timeout because we need to detect if another navigator mounted in the meantime - // For example, if another navigator has started rendering, we should skip cleanup - // Otherwise, our cleanup step will cleanup state for the other navigator and re-initialize it - setTimeout(() => { - if (getCurrentState() !== undefined && getKey() === navigatorKey) { - cleanUpState(); - } - }, 0); + if (getCurrentState() !== undefined && getKey() === navigatorKey) { + setCurrentState(undefined); + stateCleanedUp.current = true; + } }; // eslint-disable-next-line react-hooks/exhaustive-deps }, []); - // We initialize this ref here to avoid a new getState getting initialized - // whenever initializedState changes. We want getState to have access to the - // latest initializedState, but don't need it to change when that happens - const initializedStateRef = React.useRef(); - initializedStateRef.current = initializedState; - - const getState = React.useCallback((): State => { - const currentState = getCurrentState(); + const getState = useLatestCallback((): State => { + const currentState = shouldUpdate ? nextState : getCurrentState(); - return isStateInitialized(currentState) - ? (currentState as State) - : (initializedStateRef.current as State); - }, [getCurrentState, isStateInitialized]); + return ( + isStateInitialized(currentState) ? currentState : initializedState + ) as State; + }); const emitter = useEventEmitter>((e) => { const routeNames = []; diff --git a/packages/core/src/useScheduleUpdate.tsx b/packages/core/src/useScheduleUpdate.tsx deleted file mode 100644 index a7f1334f5d..0000000000 --- a/packages/core/src/useScheduleUpdate.tsx +++ /dev/null @@ -1,32 +0,0 @@ -import * as React from 'react'; - -const MISSING_CONTEXT_ERROR = "Couldn't find a schedule context."; - -export const ScheduleUpdateContext = React.createContext<{ - scheduleUpdate: (callback: () => void) => void; - flushUpdates: () => void; -}>({ - scheduleUpdate() { - throw new Error(MISSING_CONTEXT_ERROR); - }, - flushUpdates() { - throw new Error(MISSING_CONTEXT_ERROR); - }, -}); - -/** - * When screen config changes, we want to update the navigator in the same update phase. - * However, navigation state is in the root component and React won't let us update it from a child. - * This is a workaround for that, the scheduled update is stored in the ref without actually calling setState. - * It lets all subsequent updates access the latest state so it stays correct. - * Then we call setState during after the component updates. - */ -export function useScheduleUpdate(callback: () => void) { - const { scheduleUpdate, flushUpdates } = React.useContext( - ScheduleUpdateContext - ); - - scheduleUpdate(callback); - - React.useEffect(flushUpdates); -} diff --git a/packages/core/src/useSyncState.tsx b/packages/core/src/useSyncState.tsx index cdab94cc83..f085cce46c 100644 --- a/packages/core/src/useSyncState.tsx +++ b/packages/core/src/useSyncState.tsx @@ -1,74 +1,46 @@ import * as React from 'react'; -const UNINTIALIZED_STATE = {}; +const createStore = (getInitialState: () => T) => { + const listeners: (() => void)[] = []; -/** - * This is definitely not compatible with concurrent mode, but we don't have a solution for sync state yet. - */ -export function useSyncState(initialState?: (() => T) | T) { - const stateRef = React.useRef(UNINTIALIZED_STATE as any); - const isSchedulingRef = React.useRef(false); - const isMountedRef = React.useRef(true); + let state: T = getInitialState(); - React.useEffect(() => { - isMountedRef.current = true; + const getState = () => state; - return () => { - isMountedRef.current = false; - }; - }, []); - - if (stateRef.current === UNINTIALIZED_STATE) { - stateRef.current = - // @ts-expect-error: initialState is a function, but TypeScript doesn't think so - typeof initialState === 'function' ? initialState() : initialState; - } - - const [trackingState, setTrackingState] = React.useState(stateRef.current); - - const getState = React.useCallback(() => stateRef.current, []); + const setState = (newState: T) => { + state = newState; + listeners.forEach((listener) => listener()); + }; - const setState = React.useCallback((state: T) => { - if (state === stateRef.current || !isMountedRef.current) { - return; - } + const subscribe = (callback: () => void) => { + listeners.push(callback); - stateRef.current = state; - - if (!isSchedulingRef.current) { - setTrackingState(state); - } - }, []); - - const scheduleUpdate = React.useCallback((callback: () => void) => { - isSchedulingRef.current = true; - - try { - callback(); - } finally { - isSchedulingRef.current = false; - } - }, []); + return () => { + const index = listeners.indexOf(callback); - const flushUpdates = React.useCallback(() => { - if (!isMountedRef.current) { - return; - } + if (index > -1) { + listeners.splice(index, 1); + } + }; + }; - // Make sure that the tracking state is up-to-date. - // We call it unconditionally, but React should skip the update if state is unchanged. - setTrackingState(stateRef.current); - }, []); + return { + getState, + setState, + subscribe, + }; +}; - // If we're rendering and the tracking state is out of date, update it immediately - // This will make sure that our updates are applied as early as possible. - if (trackingState !== stateRef.current) { - setTrackingState(stateRef.current); - } +export function useSyncState(getInitialState: () => T) { + const store = React.useRef(createStore(getInitialState)).current; - const state = stateRef.current; + const state = React.useSyncExternalStore( + store.subscribe, + store.getState, + store.getState + ); React.useDebugValue(state); - return [state, getState, setState, scheduleUpdate, flushUpdates] as const; + return [state, store.getState, store.setState] as const; } diff --git a/packages/native/src/__tests__/ServerContainer.test.tsx b/packages/native/src/__tests__/ServerContainer.test.tsx index 01dcedd2c4..c8fe2f3d71 100644 --- a/packages/native/src/__tests__/ServerContainer.test.tsx +++ b/packages/native/src/__tests__/ServerContainer.test.tsx @@ -23,13 +23,19 @@ window.removeEventListener = () => {}; jest.mock('../useLinking', () => require('../useLinking.tsx')); // Since Jest is configured for React Native, the *.native.js file is imported -// But as we're testing server rendering, we want to use the web version -// So we mock it to point to the web version -jest.mock( - 'use-latest-callback/lib/useIsomorphicLayoutEffect', - // eslint-disable-next-line @typescript-eslint/no-var-requires - () => require('react').useEffect -); +// Causing the wrong useIsomorphicLayoutEffect to be imported +// It causes "Warning: useLayoutEffect does nothing on the server" +// So we explicitly silence it here +// This warning is being removed in React: https://github.com/facebook/react/pull/26395 +const error = console.error; + +jest.spyOn(console, 'error').mockImplementation((...args) => { + if (/Warning: useLayoutEffect does nothing on the server/m.test(args[0])) { + return; + } + + error(...args); +}); it('renders correct state with location', () => { const createStackNavigator = createNavigatorFactory((props: any) => {