From ebab5183522f5ae03f50f88289c0e7acc208dc02 Mon Sep 17 00:00:00 2001 From: Satyajit Sahoo Date: Wed, 11 Nov 2020 22:14:27 +0100 Subject: [PATCH] fix: remove the state property from route prop BREAKING CHANGE: any code which relies on `route.state` will break. Previous versions printed a warning on accessing `route.state`. This commit removes the property entirely. Accessing this property isn't safe since child navigator state isn't gurranteed to be in sync with parent navigator state and cause subtle bugs in apps. --- .../getFocusedRouteNameFromRoute.test.tsx | 49 +++++++++++++++++++ .../core/src/getFocusedRouteNameFromRoute.tsx | 18 ++----- packages/core/src/useRouteCache.tsx | 17 ++----- 3 files changed, 59 insertions(+), 25 deletions(-) diff --git a/packages/core/src/__tests__/getFocusedRouteNameFromRoute.test.tsx b/packages/core/src/__tests__/getFocusedRouteNameFromRoute.test.tsx index e654198d..58caf9a3 100644 --- a/packages/core/src/__tests__/getFocusedRouteNameFromRoute.test.tsx +++ b/packages/core/src/__tests__/getFocusedRouteNameFromRoute.test.tsx @@ -1,4 +1,5 @@ import getFocusedRouteNameFromRoute from '../getFocusedRouteNameFromRoute'; +import { CHILD_STATE } from '../useRouteCache'; it('gets undefined if there is no nested state', () => { expect(getFocusedRouteNameFromRoute({ name: 'Home' })).toBe(undefined); @@ -8,6 +9,7 @@ it('gets focused route name from nested state', () => { expect( getFocusedRouteNameFromRoute({ name: 'Home', + // @ts-expect-error: this isn't in the type defs state: { routes: [{ name: 'Article' }], }, @@ -17,6 +19,7 @@ it('gets focused route name from nested state', () => { expect( getFocusedRouteNameFromRoute({ name: 'Home', + // @ts-expect-error: this isn't in the type defs state: { index: 1, routes: [{ name: 'Article' }, { name: 'Chat' }, { name: 'Album' }], @@ -27,6 +30,7 @@ it('gets focused route name from nested state', () => { expect( getFocusedRouteNameFromRoute({ name: 'Home', + // @ts-expect-error: this isn't in the type defs state: { routes: [{ name: 'Article' }, { name: 'Chat' }], }, @@ -36,6 +40,7 @@ it('gets focused route name from nested state', () => { expect( getFocusedRouteNameFromRoute({ name: 'Home', + // @ts-expect-error: this isn't in the type defs state: { type: 'tab', routes: [{ name: 'Article' }, { name: 'Chat' }], @@ -44,6 +49,50 @@ it('gets focused route name from nested state', () => { ).toBe('Article'); }); +it('gets focused route name from nested state with symbol', () => { + expect( + getFocusedRouteNameFromRoute({ + name: 'Home', + // @ts-expect-error: this isn't in the type defs + [CHILD_STATE]: { + routes: [{ name: 'Article' }], + }, + }) + ).toBe('Article'); + + expect( + getFocusedRouteNameFromRoute({ + name: 'Home', + // @ts-expect-error: this isn't in the type defs + [CHILD_STATE]: { + index: 1, + routes: [{ name: 'Article' }, { name: 'Chat' }, { name: 'Album' }], + }, + }) + ).toBe('Chat'); + + expect( + getFocusedRouteNameFromRoute({ + name: 'Home', + // @ts-expect-error: this isn't in the type defs + [CHILD_STATE]: { + routes: [{ name: 'Article' }, { name: 'Chat' }], + }, + }) + ).toBe('Chat'); + + expect( + getFocusedRouteNameFromRoute({ + name: 'Home', + // @ts-expect-error: this isn't in the type defs + [CHILD_STATE]: { + type: 'tab', + routes: [{ name: 'Article' }, { name: 'Chat' }], + }, + }) + ).toBe('Article'); +}); + it('gets nested screen in params if present', () => { expect( getFocusedRouteNameFromRoute({ diff --git a/packages/core/src/getFocusedRouteNameFromRoute.tsx b/packages/core/src/getFocusedRouteNameFromRoute.tsx index 8fc661b6..9038fc2e 100644 --- a/packages/core/src/getFocusedRouteNameFromRoute.tsx +++ b/packages/core/src/getFocusedRouteNameFromRoute.tsx @@ -1,19 +1,11 @@ -import type { - Route, - PartialState, - NavigationState, -} from '@react-navigation/routers'; -import { SUPPRESS_STATE_ACCESS_WARNING } from './useRouteCache'; +import type { Route } from '@react-navigation/routers'; +import { CHILD_STATE } from './useRouteCache'; export default function getFocusedRouteNameFromRoute( - route: Partial> & { state?: PartialState } + route: Partial> ): string | undefined { - SUPPRESS_STATE_ACCESS_WARNING.value = true; - - const state = route.state; - - SUPPRESS_STATE_ACCESS_WARNING.value = false; - + // @ts-expect-error: this isn't in type definitions coz we want this private + const state = route[CHILD_STATE] ?? route.state; const params = route.params as { screen?: unknown } | undefined; const routeName = state diff --git a/packages/core/src/useRouteCache.tsx b/packages/core/src/useRouteCache.tsx index 1dfd23dd..ca53abc1 100644 --- a/packages/core/src/useRouteCache.tsx +++ b/packages/core/src/useRouteCache.tsx @@ -13,7 +13,7 @@ type RouteCache = Map, RouteProp>; * So we need a way to suppress the warning for those use cases. * This is fine since they are internal utilities and this is not public API. */ -export const SUPPRESS_STATE_ACCESS_WARNING = { value: false }; +export const CHILD_STATE = Symbol('CHILD_STATE'); /** * Hook to cache route props for each screen in the navigator. @@ -37,18 +37,11 @@ export default function useRouteCache( // If a cached route object already exists, reuse it acc.set(route, previous); } else { - const proxy = { ...route }; + const { state, ...proxy } = route; - Object.defineProperty(proxy, 'state', { - get() { - if (!SUPPRESS_STATE_ACCESS_WARNING.value) { - console.warn( - "Accessing the 'state' property of the 'route' object is not supported. If you want to get the focused route name, use the 'getFocusedRouteNameFromRoute' helper instead: https://reactnavigation.org/docs/screen-options-resolution/#setting-parent-screen-options-based-on-child-navigators-state" - ); - } - - return route.state; - }, + Object.defineProperty(proxy, CHILD_STATE, { + enumerable: false, + value: state, }); acc.set(route, proxy);