From 5fc443e994384194131c7b8dcfca674b84f3f189 Mon Sep 17 00:00:00 2001 From: Mark Lawlor Date: Tue, 26 Mar 2024 15:11:20 +1000 Subject: [PATCH] [router] fix relative navigation on hoisted routes (#27778) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Why Fix #27753 # How This `isIndexPath` function was not taking in account hoisted index routes that may not have the `param` structure Take the following structure. ``` _layout // Stack parent/ └── child/ ├── index // name: parent/child/index └── page // name: parent/child/page ``` Because there are no route params or nested `_layout`, all routes are hoisted to the root `_layout`. The existing logic for only checking for `route.params` is insufficient as it will not exist. Additionally we cannot check for `name: 'index'` as hoisted routes have complex names. # Test Plan I think our existing tests are fine, just added one for this scenario # Checklist - [ ] Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md). - [ ] Conforms with the [Documentation Writing Style Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md) - [ ] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin). --------- Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com> --- packages/expo-router/CHANGELOG.md | 1 + .../build/LocationProvider.d.ts.map | 2 +- .../expo-router/build/LocationProvider.js | 10 +++++++++- .../expo-router/build/LocationProvider.js.map | 2 +- packages/expo-router/src/LocationProvider.tsx | 13 +++++++++++- .../src/__tests__/navigation.test.ios.tsx | 20 ++++++++++++++++++- 6 files changed, 43 insertions(+), 5 deletions(-) diff --git a/packages/expo-router/CHANGELOG.md b/packages/expo-router/CHANGELOG.md index 02b404abee59c..2546253fbf018 100644 --- a/packages/expo-router/CHANGELOG.md +++ b/packages/expo-router/CHANGELOG.md @@ -31,6 +31,7 @@ - Export `toHaveRouterState` and other matcher types from `expo-router/testing-library` ([#27646](https://github.com/expo/expo/pull/27646) by [@marklawlor](https://github.com/marklawlor)) - Fix missing types from typed routes ([#27412](https://github.com/expo/expo/pull/27412) by [@marklawlor](https://github.com/marklawlor)) - Fork NavigationContainer on web to use custom linking context ([#27712](https://github.com/expo/expo/pull/27712) by [@marklawlor](https://github.com/marklawlor)) +- Fix relative navigation on hoisted routes ([#27778](https://github.com/expo/expo/pull/27778) by [@marklawlor](https://github.com/marklawlor)) ### 💡 Others diff --git a/packages/expo-router/build/LocationProvider.d.ts.map b/packages/expo-router/build/LocationProvider.d.ts.map index 1ccfa00e43bf3..4ebc688d5b1ba 100644 --- a/packages/expo-router/build/LocationProvider.d.ts.map +++ b/packages/expo-router/build/LocationProvider.d.ts.map @@ -1 +1 @@ -{"version":3,"file":"LocationProvider.d.ts","sourceRoot":"","sources":["../src/LocationProvider.tsx"],"names":[],"mappings":"AAAA,OAAO,KAAK,EAAE,KAAK,EAAE,MAAM,yBAAyB,CAAC;AAGrD,KAAK,YAAY,GAAG,MAAM,CAAC,MAAM,EAAE,MAAM,GAAG,MAAM,EAAE,CAAC,CAAC;AAEtD,MAAM,MAAM,SAAS,GAAG;IACtB,mBAAmB,EAAE,MAAM,CAAC;IAC5B,QAAQ,EAAE,MAAM,CAAC;IACjB,QAAQ,CAAC,MAAM,EAAE,YAAY,CAAC;IAC9B,QAAQ,EAAE,MAAM,EAAE,CAAC;IACnB,OAAO,EAAE,OAAO,CAAC;CAClB,CAAC;AAEF,wBAAgB,qBAAqB,CACnC,gBAAgB,EAAE,CAAC,KAAK,EAAE,KAAK,EAAE,MAAM,EAAE,OAAO,KAAK;IAAE,IAAI,EAAE,MAAM,CAAC;IAAC,MAAM,EAAE,GAAG,CAAA;CAAE,EAClF,KAAK,EAAE,KAAK,EACZ,OAAO,CAAC,EAAE,MAAM,GACf,SAAS,CAWX;AAeD,wBAAgB,sBAAsB,CACpC,EACE,IAAI,EAAE,SAAS,EACf,MAAM,GACP,EAAE;IACD,IAAI,EAAE,MAAM,CAAC;IACb,MAAM,EAAE,GAAG,CAAC;CACb,EACD,OAAO,CAAC,EAAE,MAAM,GACf,IAAI,CAAC,SAAS,EAAE,UAAU,GAAG,QAAQ,CAAC,CA0BxC"} \ No newline at end of file +{"version":3,"file":"LocationProvider.d.ts","sourceRoot":"","sources":["../src/LocationProvider.tsx"],"names":[],"mappings":"AAAA,OAAO,KAAK,EAAE,KAAK,EAAE,MAAM,yBAAyB,CAAC;AAGrD,KAAK,YAAY,GAAG,MAAM,CAAC,MAAM,EAAE,MAAM,GAAG,MAAM,EAAE,CAAC,CAAC;AAEtD,MAAM,MAAM,SAAS,GAAG;IACtB,mBAAmB,EAAE,MAAM,CAAC;IAC5B,QAAQ,EAAE,MAAM,CAAC;IACjB,QAAQ,CAAC,MAAM,EAAE,YAAY,CAAC;IAC9B,QAAQ,EAAE,MAAM,EAAE,CAAC;IACnB,OAAO,EAAE,OAAO,CAAC;CAClB,CAAC;AAEF,wBAAgB,qBAAqB,CACnC,gBAAgB,EAAE,CAAC,KAAK,EAAE,KAAK,EAAE,MAAM,EAAE,OAAO,KAAK;IAAE,IAAI,EAAE,MAAM,CAAC;IAAC,MAAM,EAAE,GAAG,CAAA;CAAE,EAClF,KAAK,EAAE,KAAK,EACZ,OAAO,CAAC,EAAE,MAAM,GACf,SAAS,CAWX;AA0BD,wBAAgB,sBAAsB,CACpC,EACE,IAAI,EAAE,SAAS,EACf,MAAM,GACP,EAAE;IACD,IAAI,EAAE,MAAM,CAAC;IACb,MAAM,EAAE,GAAG,CAAC;CACb,EACD,OAAO,CAAC,EAAE,MAAM,GACf,IAAI,CAAC,SAAS,EAAE,UAAU,GAAG,QAAQ,CAAC,CA0BxC"} \ No newline at end of file diff --git a/packages/expo-router/build/LocationProvider.js b/packages/expo-router/build/LocationProvider.js index 23f8f43547f97..8d485b5d6a9af 100644 --- a/packages/expo-router/build/LocationProvider.js +++ b/packages/expo-router/build/LocationProvider.js @@ -19,10 +19,18 @@ function isIndexPath(state) { if (route.state) { return isIndexPath(route.state); } - // router.params is typed as 'object', so this usual syntax is to please TypeScript + // Index routes on the same level as a layout do not have `index` in their name if (route.params && 'screen' in route.params) { return route.params.screen === 'index'; } + // The `params` key will not exist if there are no params + // So we need to do a positive lookahead to check if the route ends with /index + // Nested routes that are hoisted will have a name ending with /index + // e.g name could be /user/[id]/index + if (route.name.match(/.+\/index$/)) + return true; + // The state will either have params (because there are multiple _layout) or it will be hoisted with a name + // If we don't match the above cases, then it's not an index route return false; } // TODO: Split up getPathFromState to return all this info at once. diff --git a/packages/expo-router/build/LocationProvider.js.map b/packages/expo-router/build/LocationProvider.js.map index 77e0b6b40c553..56de5fbd030bd 100644 --- a/packages/expo-router/build/LocationProvider.js.map +++ b/packages/expo-router/build/LocationProvider.js.map @@ -1 +1 @@ -{"version":3,"file":"LocationProvider.js","sourceRoot":"","sources":["../src/LocationProvider.tsx"],"names":[],"mappings":";;;AACA,8DAAuD;AAYvD,SAAgB,qBAAqB,CACnC,gBAAkF,EAClF,KAAY,EACZ,OAAgB;IAEhB,MAAM,EAAE,IAAI,EAAE,GAAG,gBAAgB,CAAC,KAAK,EAAE,KAAK,CAAC,CAAC;IAChD,MAAM,SAAS,GAAG,gBAAgB,CAAC,KAAK,EAAE,IAAI,CAAC,CAAC;IAEhD,OAAO;QACL,kEAAkE;QAClE,mBAAmB,EAAE,IAAI;QACzB,QAAQ,EAAE,IAAA,+BAAY,EAAC,IAAI,EAAE,OAAO,CAAC,CAAC,KAAK,CAAC,GAAG,CAAC,CAAC,GAAG,CAAC;QACrD,OAAO,EAAE,WAAW,CAAC,KAAK,CAAC;QAC3B,GAAG,sBAAsB,CAAC,SAAS,EAAE,OAAO,CAAC;KAC9C,CAAC;AACJ,CAAC;AAfD,sDAeC;AAED,SAAS,WAAW,CAAC,KAAY;IAC/B,MAAM,KAAK,GAAG,KAAK,CAAC,MAAM,CAAC,KAAK,CAAC,KAAK,IAAI,KAAK,CAAC,MAAM,CAAC,MAAM,GAAG,CAAC,CAAC,CAAC;IACnE,IAAI,KAAK,CAAC,KAAK,EAAE;QACf,OAAO,WAAW,CAAC,KAAK,CAAC,KAAK,CAAC,CAAC;KACjC;IACD,mFAAmF;IACnF,IAAI,KAAK,CAAC,MAAM,IAAI,QAAQ,IAAI,KAAK,CAAC,MAAM,EAAE;QAC5C,OAAO,KAAK,CAAC,MAAM,CAAC,MAAM,KAAK,OAAO,CAAC;KACxC;IACD,OAAO,KAAK,CAAC;AACf,CAAC;AAED,mEAAmE;AACnE,SAAgB,sBAAsB,CACpC,EACE,IAAI,EAAE,SAAS,EACf,MAAM,GAIP,EACD,OAAgB;IAEhB,MAAM,CAAC,QAAQ,CAAC,GAAG,SAAS,CAAC,KAAK,CAAC,GAAG,CAAC,CAAC;IACxC,OAAO;QACL,gCAAgC;QAChC,QAAQ,EAAE,IAAA,+BAAY,EAAC,QAAQ,EAAE,OAAO,CAAC,CAAC,KAAK,CAAC,GAAG,CAAC,CAAC,MAAM,CAAC,OAAO,CAAC,CAAC,GAAG,CAAC,kBAAkB,CAAC;QAC5F,6EAA6E;QAC7E,8CAA8C;QAC9C,MAAM,EAAE,MAAM,CAAC,OAAO,CAAC,MAAM,CAAC,CAAC,MAAM,CAAC,CAAC,IAAI,EAAE,CAAC,GAAG,EAAE,KAAK,CAAC,EAAE,EAAE;YAC3D,IAAI,KAAK,CAAC,OAAO,CAAC,KAAK,CAAC,EAAE;gBACxB,IAAI,CAAC,GAAG,CAAC,GAAG,KAAK,CAAC,GAAG,CAAC,CAAC,CAAS,EAAE,EAAE;oBAClC,IAAI;wBACF,OAAO,kBAAkB,CAAC,CAAC,CAAC,CAAC;qBAC9B;oBAAC,MAAM;wBACN,OAAO,CAAC,CAAC;qBACV;gBACH,CAAC,CAAC,CAAC;aACJ;iBAAM;gBACL,IAAI;oBACF,IAAI,CAAC,GAAG,CAAC,GAAG,kBAAkB,CAAC,KAAe,CAAC,CAAC;iBACjD;gBAAC,MAAM;oBACN,IAAI,CAAC,GAAG,CAAC,GAAG,KAAe,CAAC;iBAC7B;aACF;YACD,OAAO,IAAI,CAAC;QACd,CAAC,EAAE,EAAkB,CAAC;KACvB,CAAC;AACJ,CAAC;AAnCD,wDAmCC","sourcesContent":["import type { State } from './fork/getPathFromState';\nimport { stripBaseUrl } from './fork/getStateFromPath';\n\ntype SearchParams = Record;\n\nexport type UrlObject = {\n unstable_globalHref: string;\n pathname: string;\n readonly params: SearchParams;\n segments: string[];\n isIndex: boolean;\n};\n\nexport function getRouteInfoFromState(\n getPathFromState: (state: State, asPath: boolean) => { path: string; params: any },\n state: State,\n baseUrl?: string\n): UrlObject {\n const { path } = getPathFromState(state, false);\n const qualified = getPathFromState(state, true);\n\n return {\n // TODO: This may have a predefined origin attached in the future.\n unstable_globalHref: path,\n pathname: stripBaseUrl(path, baseUrl).split('?')['0'],\n isIndex: isIndexPath(state),\n ...getNormalizedStatePath(qualified, baseUrl),\n };\n}\n\nfunction isIndexPath(state: State) {\n const route = state.routes[state.index ?? state.routes.length - 1];\n if (route.state) {\n return isIndexPath(route.state);\n }\n // router.params is typed as 'object', so this usual syntax is to please TypeScript\n if (route.params && 'screen' in route.params) {\n return route.params.screen === 'index';\n }\n return false;\n}\n\n// TODO: Split up getPathFromState to return all this info at once.\nexport function getNormalizedStatePath(\n {\n path: statePath,\n params,\n }: {\n path: string;\n params: any;\n },\n baseUrl?: string\n): Pick {\n const [pathname] = statePath.split('?');\n return {\n // Strip empty path at the start\n segments: stripBaseUrl(pathname, baseUrl).split('/').filter(Boolean).map(decodeURIComponent),\n // TODO: This is not efficient, we should generate based on the state instead\n // of converting to string then back to object\n params: Object.entries(params).reduce((prev, [key, value]) => {\n if (Array.isArray(value)) {\n prev[key] = value.map((v: string) => {\n try {\n return decodeURIComponent(v);\n } catch {\n return v;\n }\n });\n } else {\n try {\n prev[key] = decodeURIComponent(value as string);\n } catch {\n prev[key] = value as string;\n }\n }\n return prev;\n }, {} as SearchParams),\n };\n}\n"]} \ No newline at end of file +{"version":3,"file":"LocationProvider.js","sourceRoot":"","sources":["../src/LocationProvider.tsx"],"names":[],"mappings":";;;AACA,8DAAuD;AAYvD,SAAgB,qBAAqB,CACnC,gBAAkF,EAClF,KAAY,EACZ,OAAgB;IAEhB,MAAM,EAAE,IAAI,EAAE,GAAG,gBAAgB,CAAC,KAAK,EAAE,KAAK,CAAC,CAAC;IAChD,MAAM,SAAS,GAAG,gBAAgB,CAAC,KAAK,EAAE,IAAI,CAAC,CAAC;IAEhD,OAAO;QACL,kEAAkE;QAClE,mBAAmB,EAAE,IAAI;QACzB,QAAQ,EAAE,IAAA,+BAAY,EAAC,IAAI,EAAE,OAAO,CAAC,CAAC,KAAK,CAAC,GAAG,CAAC,CAAC,GAAG,CAAC;QACrD,OAAO,EAAE,WAAW,CAAC,KAAK,CAAC;QAC3B,GAAG,sBAAsB,CAAC,SAAS,EAAE,OAAO,CAAC;KAC9C,CAAC;AACJ,CAAC;AAfD,sDAeC;AAED,SAAS,WAAW,CAAC,KAAY;IAC/B,MAAM,KAAK,GAAG,KAAK,CAAC,MAAM,CAAC,KAAK,CAAC,KAAK,IAAI,KAAK,CAAC,MAAM,CAAC,MAAM,GAAG,CAAC,CAAC,CAAC;IACnE,IAAI,KAAK,CAAC,KAAK,EAAE;QACf,OAAO,WAAW,CAAC,KAAK,CAAC,KAAK,CAAC,CAAC;KACjC;IAED,+EAA+E;IAC/E,IAAI,KAAK,CAAC,MAAM,IAAI,QAAQ,IAAI,KAAK,CAAC,MAAM,EAAE;QAC5C,OAAO,KAAK,CAAC,MAAM,CAAC,MAAM,KAAK,OAAO,CAAC;KACxC;IAED,yDAAyD;IACzD,+EAA+E;IAC/E,qEAAqE;IACrE,qCAAqC;IACrC,IAAI,KAAK,CAAC,IAAI,CAAC,KAAK,CAAC,YAAY,CAAC;QAAE,OAAO,IAAI,CAAC;IAEhD,2GAA2G;IAC3G,kEAAkE;IAElE,OAAO,KAAK,CAAC;AACf,CAAC;AAED,mEAAmE;AACnE,SAAgB,sBAAsB,CACpC,EACE,IAAI,EAAE,SAAS,EACf,MAAM,GAIP,EACD,OAAgB;IAEhB,MAAM,CAAC,QAAQ,CAAC,GAAG,SAAS,CAAC,KAAK,CAAC,GAAG,CAAC,CAAC;IACxC,OAAO;QACL,gCAAgC;QAChC,QAAQ,EAAE,IAAA,+BAAY,EAAC,QAAQ,EAAE,OAAO,CAAC,CAAC,KAAK,CAAC,GAAG,CAAC,CAAC,MAAM,CAAC,OAAO,CAAC,CAAC,GAAG,CAAC,kBAAkB,CAAC;QAC5F,6EAA6E;QAC7E,8CAA8C;QAC9C,MAAM,EAAE,MAAM,CAAC,OAAO,CAAC,MAAM,CAAC,CAAC,MAAM,CAAC,CAAC,IAAI,EAAE,CAAC,GAAG,EAAE,KAAK,CAAC,EAAE,EAAE;YAC3D,IAAI,KAAK,CAAC,OAAO,CAAC,KAAK,CAAC,EAAE;gBACxB,IAAI,CAAC,GAAG,CAAC,GAAG,KAAK,CAAC,GAAG,CAAC,CAAC,CAAS,EAAE,EAAE;oBAClC,IAAI;wBACF,OAAO,kBAAkB,CAAC,CAAC,CAAC,CAAC;qBAC9B;oBAAC,MAAM;wBACN,OAAO,CAAC,CAAC;qBACV;gBACH,CAAC,CAAC,CAAC;aACJ;iBAAM;gBACL,IAAI;oBACF,IAAI,CAAC,GAAG,CAAC,GAAG,kBAAkB,CAAC,KAAe,CAAC,CAAC;iBACjD;gBAAC,MAAM;oBACN,IAAI,CAAC,GAAG,CAAC,GAAG,KAAe,CAAC;iBAC7B;aACF;YACD,OAAO,IAAI,CAAC;QACd,CAAC,EAAE,EAAkB,CAAC;KACvB,CAAC;AACJ,CAAC;AAnCD,wDAmCC","sourcesContent":["import type { State } from './fork/getPathFromState';\nimport { stripBaseUrl } from './fork/getStateFromPath';\n\ntype SearchParams = Record;\n\nexport type UrlObject = {\n unstable_globalHref: string;\n pathname: string;\n readonly params: SearchParams;\n segments: string[];\n isIndex: boolean;\n};\n\nexport function getRouteInfoFromState(\n getPathFromState: (state: State, asPath: boolean) => { path: string; params: any },\n state: State,\n baseUrl?: string\n): UrlObject {\n const { path } = getPathFromState(state, false);\n const qualified = getPathFromState(state, true);\n\n return {\n // TODO: This may have a predefined origin attached in the future.\n unstable_globalHref: path,\n pathname: stripBaseUrl(path, baseUrl).split('?')['0'],\n isIndex: isIndexPath(state),\n ...getNormalizedStatePath(qualified, baseUrl),\n };\n}\n\nfunction isIndexPath(state: State) {\n const route = state.routes[state.index ?? state.routes.length - 1];\n if (route.state) {\n return isIndexPath(route.state);\n }\n\n // Index routes on the same level as a layout do not have `index` in their name\n if (route.params && 'screen' in route.params) {\n return route.params.screen === 'index';\n }\n\n // The `params` key will not exist if there are no params\n // So we need to do a positive lookahead to check if the route ends with /index\n // Nested routes that are hoisted will have a name ending with /index\n // e.g name could be /user/[id]/index\n if (route.name.match(/.+\\/index$/)) return true;\n\n // The state will either have params (because there are multiple _layout) or it will be hoisted with a name\n // If we don't match the above cases, then it's not an index route\n\n return false;\n}\n\n// TODO: Split up getPathFromState to return all this info at once.\nexport function getNormalizedStatePath(\n {\n path: statePath,\n params,\n }: {\n path: string;\n params: any;\n },\n baseUrl?: string\n): Pick {\n const [pathname] = statePath.split('?');\n return {\n // Strip empty path at the start\n segments: stripBaseUrl(pathname, baseUrl).split('/').filter(Boolean).map(decodeURIComponent),\n // TODO: This is not efficient, we should generate based on the state instead\n // of converting to string then back to object\n params: Object.entries(params).reduce((prev, [key, value]) => {\n if (Array.isArray(value)) {\n prev[key] = value.map((v: string) => {\n try {\n return decodeURIComponent(v);\n } catch {\n return v;\n }\n });\n } else {\n try {\n prev[key] = decodeURIComponent(value as string);\n } catch {\n prev[key] = value as string;\n }\n }\n return prev;\n }, {} as SearchParams),\n };\n}\n"]} \ No newline at end of file diff --git a/packages/expo-router/src/LocationProvider.tsx b/packages/expo-router/src/LocationProvider.tsx index 9b3227919c0a5..85bfe56d3059b 100644 --- a/packages/expo-router/src/LocationProvider.tsx +++ b/packages/expo-router/src/LocationProvider.tsx @@ -33,10 +33,21 @@ function isIndexPath(state: State) { if (route.state) { return isIndexPath(route.state); } - // router.params is typed as 'object', so this usual syntax is to please TypeScript + + // Index routes on the same level as a layout do not have `index` in their name if (route.params && 'screen' in route.params) { return route.params.screen === 'index'; } + + // The `params` key will not exist if there are no params + // So we need to do a positive lookahead to check if the route ends with /index + // Nested routes that are hoisted will have a name ending with /index + // e.g name could be /user/[id]/index + if (route.name.match(/.+\/index$/)) return true; + + // The state will either have params (because there are multiple _layout) or it will be hoisted with a name + // If we don't match the above cases, then it's not an index route + return false; } diff --git a/packages/expo-router/src/__tests__/navigation.test.ios.tsx b/packages/expo-router/src/__tests__/navigation.test.ios.tsx index ecbf5393a3eea..c87ec5d3b0c53 100644 --- a/packages/expo-router/src/__tests__/navigation.test.ios.tsx +++ b/packages/expo-router/src/__tests__/navigation.test.ios.tsx @@ -1,5 +1,5 @@ /* eslint-disable react-hooks/rules-of-hooks */ -import React, { Text } from 'react-native'; +import React, { Text, View } from 'react-native'; import { useRouter, @@ -9,6 +9,7 @@ import { Redirect, Slot, usePathname, + Link, } from '../exports'; import { Stack } from '../layouts/Stack'; import { Tabs } from '../layouts/Tabs'; @@ -821,6 +822,23 @@ it('can push relative links from index routes', async () => { expect(screen).toHavePathname('/test/bar'); }); +it('can push relative links from hoisted routes', () => { + renderRouter( + { + _layout: () => , + 'parent/index': () => , + 'parent/child': () => , + }, + { + initialUrl: '/parent', + } + ); + + expect(screen.getByTestId('link')).toBeOnTheScreen(); + fireEvent(screen.getByTestId('link'), 'press'); + expect(screen.getByTestId('child')).toBeOnTheScreen(); +}); + it('can navigation to a relative route without losing path params', async () => { renderRouter( {