From 366d0181dc02597b8d11e343f37fc77bee164c70 Mon Sep 17 00:00:00 2001 From: Satyajit Sahoo Date: Wed, 11 Nov 2020 23:11:31 +0100 Subject: [PATCH] fix: don't merge params on navigation BREAKING CHANGE: Previous versions of React Navigation merged params on navigation which caused confusion. This commit changes params not to be merged. The old behaviour can still be achieved by passing `merge: true` explicitly: ```js CommonActions.navigate({ name: 'bar', params: { fruit: 'orange' }, merge: true, }) ``` `initialParams` specified for the screen are always merged. --- packages/core/src/useNavigationBuilder.tsx | 3 +- packages/routers/src/CommonActions.tsx | 6 +- packages/routers/src/StackRouter.tsx | 19 +- packages/routers/src/TabRouter.tsx | 57 +++-- .../src/__tests__/StackRouter.test.tsx | 67 +++--- .../routers/src/__tests__/TabRouter.test.tsx | 203 ++++++++---------- 6 files changed, 172 insertions(+), 183 deletions(-) diff --git a/packages/core/src/useNavigationBuilder.tsx b/packages/core/src/useNavigationBuilder.tsx index 3d5b0c21..268c8de9 100644 --- a/packages/core/src/useNavigationBuilder.tsx +++ b/packages/core/src/useNavigationBuilder.tsx @@ -353,7 +353,7 @@ export default function useNavigationBuilder< if ( typeof route.params.state === 'object' && route.params.state != null && - route.params.state !== previousParams?.state + route.params !== previousParams ) { // If the route was updated with new state, we should reset to it action = CommonActions.reset(route.params.state); @@ -362,7 +362,6 @@ export default function useNavigationBuilder< ((route.params.initial === false && isFirstStateInitialization) || route.params !== previousParams) ) { - // FIXME: Since params are merged, `route.params.params` might contain params from an older route // If the route was updated with new screen name and/or params, we should navigate there action = CommonActions.navigate(route.params.screen, route.params.params); } diff --git a/packages/routers/src/CommonActions.tsx b/packages/routers/src/CommonActions.tsx index e4fccf22..886888cc 100644 --- a/packages/routers/src/CommonActions.tsx +++ b/packages/routers/src/CommonActions.tsx @@ -39,9 +39,9 @@ export function goBack(): Action { } export function navigate( - route: - | { key: string; params?: object } - | { name: string; key?: string; params?: object } + options: + | { key: string; params?: object; merge?: boolean } + | { name: string; key?: string; params?: object; merge?: boolean } ): Action; // eslint-disable-next-line no-redeclare export function navigate(name: string, params?: object): Action; diff --git a/packages/routers/src/StackRouter.tsx b/packages/routers/src/StackRouter.tsx index 42e62a27..0523bf95 100644 --- a/packages/routers/src/StackRouter.tsx +++ b/packages/routers/src/StackRouter.tsx @@ -401,7 +401,17 @@ export default function StackRouter(options: StackRouterOptions) { let params; - if (action.payload.merge === false) { + if (action.payload.merge) { + params = + action.payload.params !== undefined || + routeParamList[route.name] !== undefined + ? { + ...routeParamList[route.name], + ...route.params, + ...action.payload.params, + } + : route.params; + } else { params = routeParamList[route.name] !== undefined ? { @@ -409,13 +419,6 @@ export default function StackRouter(options: StackRouterOptions) { ...action.payload.params, } : action.payload.params; - } else { - params = action.payload.params - ? { - ...route.params, - ...action.payload.params, - } - : route.params; } return { diff --git a/packages/routers/src/TabRouter.tsx b/packages/routers/src/TabRouter.tsx index bfb6eff7..3a9a92f5 100644 --- a/packages/routers/src/TabRouter.tsx +++ b/packages/routers/src/TabRouter.tsx @@ -294,40 +294,35 @@ export default function TabRouter({ return changeIndex( { ...state, - routes: - action.payload.params !== undefined - ? state.routes.map((route, i) => { - if (i !== index) { - return route; - } + routes: state.routes.map((route, i) => { + if (i !== index) { + return route; + } - let params; + let params; - if ( - action.type === 'NAVIGATE' && - action.payload.merge === false - ) { - params = - routeParamList[route.name] !== undefined - ? { - ...routeParamList[route.name], - ...action.payload.params, - } - : action.payload.params; - } else { - params = action.payload.params - ? { - ...route.params, - ...action.payload.params, - } - : route.params; - } + if (action.type === 'NAVIGATE' && action.payload.merge) { + params = + action.payload.params !== undefined || + routeParamList[route.name] !== undefined + ? { + ...routeParamList[route.name], + ...route.params, + ...action.payload.params, + } + : route.params; + } else { + params = + routeParamList[route.name] !== undefined + ? { + ...routeParamList[route.name], + ...action.payload.params, + } + : action.payload.params; + } - return params !== route.params - ? { ...route, params } - : route; - }) - : state.routes, + return params !== route.params ? { ...route, params } : route; + }), }, index, backBehavior, diff --git a/packages/routers/src/__tests__/StackRouter.test.tsx b/packages/routers/src/__tests__/StackRouter.test.tsx index fd90e8d7..f71fe709 100644 --- a/packages/routers/src/__tests__/StackRouter.test.tsx +++ b/packages/routers/src/__tests__/StackRouter.test.tsx @@ -1010,11 +1010,13 @@ it('changes index on focus change', () => { expect(router.getStateForRouteFocus(state, 'qux-0')).toEqual(state); }); -it('merges params on navigate to an existing screen', () => { +it("doesn't merge params on navigate to an existing screen", () => { const router = StackRouter({}); const options = { routeNames: ['baz', 'bar', 'qux'], - routeParamList: {}, + routeParamList: { + bar: { color: 'test' }, + }, }; expect( @@ -1042,7 +1044,7 @@ it('merges params on navigate to an existing screen', () => { routeNames: ['baz', 'bar', 'qux'], routes: [ { key: 'baz', name: 'baz' }, - { key: 'bar', name: 'bar', params: { answer: 42 } }, + { key: 'bar', name: 'bar', params: { color: 'test' } }, ], }); @@ -1070,16 +1072,17 @@ it('merges params on navigate to an existing screen', () => { routeNames: ['baz', 'bar', 'qux'], routes: [ { key: 'baz', name: 'baz' }, - { key: 'bar', name: 'bar', params: { answer: 42, fruit: 'orange' } }, + { key: 'bar', name: 'bar', params: { color: 'test', fruit: 'orange' } }, ], }); }); -it("doesn't merge params on navigate to an existing screen if merge: false", () => { +it('merges params on navigate to an existing screen if merge: true', () => { const router = StackRouter({}); const options = { routeNames: ['baz', 'bar', 'qux'], routeParamList: { + bar: { color: 'test' }, baz: { foo: 12 }, }, }; @@ -1098,13 +1101,11 @@ it("doesn't merge params on navigate to an existing screen if merge: false", () { key: 'qux', name: 'qux' }, ], }, - { - type: 'NAVIGATE', - payload: { - name: 'bar', - merge: false, - }, - }, + + CommonActions.navigate({ + name: 'bar', + merge: true, + }), options ) ).toEqual({ @@ -1115,7 +1116,7 @@ it("doesn't merge params on navigate to an existing screen if merge: false", () routeNames: ['baz', 'bar', 'qux'], routes: [ { key: 'baz', name: 'baz' }, - { key: 'bar', name: 'bar' }, + { key: 'bar', name: 'bar', params: { color: 'test', answer: 42 } }, ], }); @@ -1132,14 +1133,11 @@ it("doesn't merge params on navigate to an existing screen if merge: false", () { key: 'bar', name: 'bar', params: { answer: 42 } }, ], }, - { - type: 'NAVIGATE', - payload: { - name: 'bar', - params: { fruit: 'orange' }, - merge: false, - }, - }, + CommonActions.navigate({ + name: 'bar', + params: { fruit: 'orange' }, + merge: true, + }), options ) ).toEqual({ @@ -1150,7 +1148,11 @@ it("doesn't merge params on navigate to an existing screen if merge: false", () routeNames: ['baz', 'bar', 'qux'], routes: [ { key: 'baz', name: 'baz' }, - { key: 'bar', name: 'bar', params: { fruit: 'orange' } }, + { + key: 'bar', + name: 'bar', + params: { color: 'test', fruit: 'orange', answer: 42 }, + }, ], }); @@ -1167,14 +1169,11 @@ it("doesn't merge params on navigate to an existing screen if merge: false", () { key: 'bar', name: 'bar', params: { answer: 42 } }, ], }, - { - type: 'NAVIGATE', - payload: { - name: 'baz', - params: { color: 'black' }, - merge: false, - }, - }, + CommonActions.navigate({ + name: 'baz', + params: { color: 'black' }, + merge: true, + }), options ) ).toEqual({ @@ -1183,6 +1182,12 @@ it("doesn't merge params on navigate to an existing screen if merge: false", () key: 'root', index: 0, routeNames: ['baz', 'bar', 'qux'], - routes: [{ key: 'baz', name: 'baz', params: { foo: 12, color: 'black' } }], + routes: [ + { + key: 'baz', + name: 'baz', + params: { foo: 12, test: 99, color: 'black' }, + }, + ], }); }); diff --git a/packages/routers/src/__tests__/TabRouter.test.tsx b/packages/routers/src/__tests__/TabRouter.test.tsx index c9fb02dc..cb81fdd4 100644 --- a/packages/routers/src/__tests__/TabRouter.test.tsx +++ b/packages/routers/src/__tests__/TabRouter.test.tsx @@ -1108,11 +1108,13 @@ it('updates route key history on focus change', () => { ]); }); -it('merges params on navigate to an existing screen', () => { +it("doesn't merge params on navigate to an existing screen", () => { const router = TabRouter({}); const options = { routeNames: ['baz', 'bar', 'qux'], - routeParamList: {}, + routeParamList: { + qux: { color: 'indigo' }, + }, }; expect( @@ -1141,93 +1143,7 @@ it('merges params on navigate to an existing screen', () => { routeNames: ['baz', 'bar', 'qux'], routes: [ { key: 'baz', name: 'baz' }, - { key: 'bar', name: 'bar', params: { answer: 42 } }, - { key: 'qux', name: 'qux' }, - ], - history: [ - { type: 'route', key: 'baz' }, - { type: 'route', key: 'bar' }, - ], - }); - - expect( - router.getStateForAction( - { - stale: false, - type: 'tab', - key: 'root', - index: 1, - routeNames: ['baz', 'bar', 'qux'], - routes: [ - { key: 'baz', name: 'baz' }, - { key: 'bar', name: 'bar', params: { answer: 42 } }, - { key: 'qux', name: 'qux' }, - ], - history: [{ type: 'route', key: 'baz' }], - }, - CommonActions.navigate('bar', { fruit: 'orange' }), - options - ) - ).toEqual({ - stale: false, - type: 'tab', - key: 'root', - index: 1, - routeNames: ['baz', 'bar', 'qux'], - routes: [ - { key: 'baz', name: 'baz' }, - { key: 'bar', name: 'bar', params: { answer: 42, fruit: 'orange' } }, - { key: 'qux', name: 'qux' }, - ], - history: [ - { type: 'route', key: 'baz' }, - { type: 'route', key: 'bar' }, - ], - }); -}); - -it("doesn't merge params on navigate to an existing screen if merge: false", () => { - const router = TabRouter({}); - const options = { - routeNames: ['baz', 'bar', 'qux'], - routeParamList: { - qux: { color: 'indigo' }, - }, - }; - - expect( - router.getStateForAction( - { - stale: false, - type: 'tab', - key: 'root', - index: 1, - routeNames: ['baz', 'bar', 'qux'], - routes: [ - { key: 'baz', name: 'baz' }, - { key: 'bar', name: 'bar', params: { answer: 42 } }, - { key: 'qux', name: 'qux' }, - ], - history: [{ type: 'route', key: 'baz' }], - }, - { - type: 'NAVIGATE', - payload: { - name: 'bar', - merge: false, - }, - }, - options - ) - ).toEqual({ - stale: false, - type: 'tab', - key: 'root', - index: 1, - routeNames: ['baz', 'bar', 'qux'], - routes: [ - { key: 'baz', name: 'baz' }, - { key: 'bar', name: 'bar', params: { answer: 42 } }, + { key: 'bar', name: 'bar' }, { key: 'qux', name: 'qux' }, ], history: [ @@ -1251,14 +1167,7 @@ it("doesn't merge params on navigate to an existing screen if merge: false", () ], history: [{ type: 'route', key: 'baz' }], }, - { - type: 'NAVIGATE', - payload: { - name: 'bar', - params: { fruit: 'orange' }, - merge: false, - }, - }, + CommonActions.navigate('bar', { fruit: 'orange' }), options ) ).toEqual({ @@ -1293,14 +1202,7 @@ it("doesn't merge params on navigate to an existing screen if merge: false", () ], history: [{ type: 'route', key: 'baz' }], }, - { - type: 'NAVIGATE', - payload: { - name: 'qux', - params: { test: 12 }, - merge: false, - }, - }, + CommonActions.navigate('qux', { test: 12 }), options ) ).toEqual({ @@ -1321,7 +1223,7 @@ it("doesn't merge params on navigate to an existing screen if merge: false", () }); }); -it('merges params on jump to an existing screen', () => { +it('merges params on navigate to an existing screen if merge: true', () => { const router = TabRouter({}); const options = { routeNames: ['baz', 'bar', 'qux'], @@ -1343,7 +1245,10 @@ it('merges params on jump to an existing screen', () => { ], history: [{ type: 'route', key: 'baz' }], }, - TabActions.jumpTo('bar'), + CommonActions.navigate({ + name: 'bar', + merge: true, + }), options ) ).toEqual({ @@ -1378,7 +1283,11 @@ it('merges params on jump to an existing screen', () => { ], history: [{ type: 'route', key: 'baz' }], }, - TabActions.jumpTo('bar', { fruit: 'orange' }), + CommonActions.navigate({ + name: 'bar', + params: { fruit: 'orange' }, + merge: true, + }), options ) ).toEqual({ @@ -1398,3 +1307,81 @@ it('merges params on jump to an existing screen', () => { ], }); }); + +it("doesn't merge params on jump to an existing screen", () => { + const router = TabRouter({}); + const options = { + routeNames: ['baz', 'bar', 'qux'], + routeParamList: {}, + }; + + expect( + router.getStateForAction( + { + stale: false, + type: 'tab', + key: 'root', + index: 1, + routeNames: ['baz', 'bar', 'qux'], + routes: [ + { key: 'baz', name: 'baz' }, + { key: 'bar', name: 'bar', params: { answer: 42 } }, + { key: 'qux', name: 'qux' }, + ], + history: [{ type: 'route', key: 'baz' }], + }, + TabActions.jumpTo('bar'), + options + ) + ).toEqual({ + stale: false, + type: 'tab', + key: 'root', + index: 1, + routeNames: ['baz', 'bar', 'qux'], + routes: [ + { key: 'baz', name: 'baz' }, + { key: 'bar', name: 'bar' }, + { key: 'qux', name: 'qux' }, + ], + history: [ + { type: 'route', key: 'baz' }, + { type: 'route', key: 'bar' }, + ], + }); + + expect( + router.getStateForAction( + { + stale: false, + type: 'tab', + key: 'root', + index: 1, + routeNames: ['baz', 'bar', 'qux'], + routes: [ + { key: 'baz', name: 'baz' }, + { key: 'bar', name: 'bar', params: { answer: 42 } }, + { key: 'qux', name: 'qux' }, + ], + history: [{ type: 'route', key: 'baz' }], + }, + TabActions.jumpTo('bar', { fruit: 'orange' }), + options + ) + ).toEqual({ + stale: false, + type: 'tab', + key: 'root', + index: 1, + routeNames: ['baz', 'bar', 'qux'], + routes: [ + { key: 'baz', name: 'baz' }, + { key: 'bar', name: 'bar', params: { fruit: 'orange' } }, + { key: 'qux', name: 'qux' }, + ], + history: [ + { type: 'route', key: 'baz' }, + { type: 'route', key: 'bar' }, + ], + }); +});