From 8d07126ada9f11cb3be2d26ab175cae20483ee74 Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Mon, 24 Jan 2022 10:50:45 -0800 Subject: [PATCH] fix(router): matchesIDs account for fixed segments and route params --- .../router/test/all/pr-24645.e2e.ts | 54 ++++++++ .../components/router/test/all/pr-24645.html | 130 ++++++++++++++++++ .../components/router/test/matching.spec.tsx | 37 ++++- core/src/components/router/utils/matching.ts | 60 ++++---- 4 files changed, 248 insertions(+), 33 deletions(-) create mode 100644 core/src/components/router/test/all/pr-24645.e2e.ts create mode 100644 core/src/components/router/test/all/pr-24645.html diff --git a/core/src/components/router/test/all/pr-24645.e2e.ts b/core/src/components/router/test/all/pr-24645.e2e.ts new file mode 100644 index 00000000000..3fb86875cd8 --- /dev/null +++ b/core/src/components/router/test/all/pr-24645.e2e.ts @@ -0,0 +1,54 @@ +import { newE2EPage } from '@stencil/core/testing'; + +test('nav.pop should update the URL', async () => { + const page = await newE2EPage({ + url: '/src/components/router/test/all/pr-24645.html?ionic:_testing=true' + }); + + const push = await page.$('#router-push'); + const pop = await page.$('#nav-pop'); + + const routes = [ + '/a', + '/a/b', + '/fixed/a/b/c', + '/routeparam/a', + '/routeparam/a/b', + ]; + + // Push all the routes. + for (const r of routes) { + await page.$eval('#route', (el: any, route) => el.value = route, r); + await push.click(); + await page.waitForChanges(); + expect(await page.url()).toMatch(new RegExp(`#${r}$`)); + } + + // Pop should restore the urls. + for (const r of routes.reverse()) { + expect(await page.url()).toMatch(new RegExp(`#${r}$`)); + await pop.click(); + await page.waitForChanges(); + } +}); + +test('nav.push should update the URL', async () => { + const page = await newE2EPage({ + url: '/src/components/router/test/all/pr-24645.html?ionic:_testing=true' + }); + + const pushX = await page.$('#nav-push-x'); + await pushX.click(); + await page.waitForChanges(); + expect(await page.url()).toMatch(/\/x$/); + + const pushFixed = await page.$('#nav-push-fixed'); + await pushFixed.click(); + await page.waitForChanges(); + expect(await page.url()).toMatch(/\/fixed\/x\/y\/z$/); + + const pushRouteParam = await page.$('#nav-push-routeparam'); + await pushRouteParam.click(); + await page.waitForChanges(); + expect(await page.url()).toMatch(/\/routeparam\/x$/); +}); \ No newline at end of file diff --git a/core/src/components/router/test/all/pr-24645.html b/core/src/components/router/test/all/pr-24645.html new file mode 100644 index 00000000000..7e3aa5acddf --- /dev/null +++ b/core/src/components/router/test/all/pr-24645.html @@ -0,0 +1,130 @@ + + + + + + Navigation Guards + + + + + + + + + + + + + + + + + + + + + + + + + ion-nav navigation tests + + + + + + + + / + /a + /a/b + /fixed/a/b/c + /routeparam/a + /routeparam/a/b + + + + router.push + + + nav.push(/x) + + + nav.push(/fixed/x/y/z) + + + nav.push(/routeparam/x) + + + nav.pop + + + + + + + + + + + + diff --git a/core/src/components/router/test/matching.spec.tsx b/core/src/components/router/test/matching.spec.tsx index 2d1da3d1f08..cd9d4486eff 100644 --- a/core/src/components/router/test/matching.spec.tsx +++ b/core/src/components/router/test/matching.spec.tsx @@ -43,11 +43,46 @@ describe('matchesIDs', () => { it('should match path with params', () => { const ids = [{ id: 'my-page', params: { s1: 'a', s2: 'b' } }]; + // The route has only parameter segments. expect(matchesIDs(ids, [{ id: 'my-page', segments: [''], params: {} }])).toBe(1); expect(matchesIDs(ids, [{ id: 'my-page', segments: [':s1'], params: {} }])).toBe(1); - expect(matchesIDs(ids, [{ id: 'my-page', segments: [':s1', ':s2'], params: {} }])).toBe(3); + expect(matchesIDs(ids, [{ id: 'my-page', segments: [':s1', ':s2'], params: {} }])).toBe(2); expect(matchesIDs(ids, [{ id: 'my-page', segments: [':s1', ':s2', ':s3'], params: {} }])).toBe(1); + + // The route has a mix of parameter and fixed segments. + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix'], params: {} }])).toBe(1); + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix', ':s1'], params: {} }])).toBe(1); + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix', ':s1', ':s2'], params: {} }])).toBe(2); + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix', ':s1', ':s2', ':s3'], params: {} }])).toBe(1); + + // The route has parameters (componentProps). + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix'], params: {s2: 'route param'} }])).toBe(1); + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix', ':s1'], params: {s2: 'route param'} }])).toBe(2); + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix', ':s1', ':s2'], params: {s2: 'route param'} }])).toBe(2); + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix', ':s1', ':s2', ':s3'], params: {s2: 'route param'} }])).toBe(1); }) + + it('should match path without params', () => { + const ids = [{ id: 'my-page', params: {} }]; + + // The route has only parameter segments. + expect(matchesIDs(ids, [{ id: 'my-page', segments: [''], params: {} }])).toBe(2); + expect(matchesIDs(ids, [{ id: 'my-page', segments: [':s1'], params: {} }])).toBe(1); + expect(matchesIDs(ids, [{ id: 'my-page', segments: [':s1', ':s2'], params: {} }])).toBe(1); + expect(matchesIDs(ids, [{ id: 'my-page', segments: [':s1', ':s2', ':s3'], params: {} }])).toBe(1); + + // The route has a mix of parameter and fixed segments. + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix'], params: {} }])).toBe(2); + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix', ':s1'], params: {} }])).toBe(1); + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix', ':s1', ':s2'], params: {} }])).toBe(1); + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix', ':s1', ':s2', ':s3'], params: {} }])).toBe(1); + + // The route has parameters (componentProps). + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix'], params: {s2: 'route param'} }])).toBe(1); + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix', ':s1'], params: {s2: 'route param'} }])).toBe(1); + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix', ':s1', ':s2'], params: {s2: 'route param'} }])).toBe(1); + expect(matchesIDs(ids, [{ id: 'my-page', segments: ['prefix', ':s1', ':s2', ':s3'], params: {s2: 'route param'} }])).toBe(1); + }) }); describe('matchesSegments', () => { diff --git a/core/src/components/router/utils/matching.ts b/core/src/components/router/utils/matching.ts index f183581c7f7..84b74df63c9 100644 --- a/core/src/components/router/utils/matching.ts +++ b/core/src/components/router/utils/matching.ts @@ -34,6 +34,14 @@ export const findRouteRedirect = (segments: string[], redirects: RouteRedirect[] return redirects.find(redirect => matchesRedirect(segments, redirect)); }; +/** + * Returns the matching score for a RouteID vs a RouteChain. + * + * The best score is returned when the id and the entry match for all the levels. + * They match when: + * - they both use the same id (component tag or tab name), + * - they both have the same parameters. + */ export const matchesIDs = (ids: Pick[], chain: RouteChain): number => { const len = Math.min(ids.length, chain.length); @@ -41,48 +49,36 @@ export const matchesIDs = (ids: Pick[], chain: RouteCh for (let i = 0; i < len; i++) { const routeId = ids[i]; - const routeChain = chain[i]; + const routeEntry = chain[i]; + // Skip results where the route id does not match the chain at the same index - if (routeId.id.toLowerCase() !== routeChain.id) { + if (routeId.id.toLowerCase() === routeEntry.id.toLowerCase()) { + score++; + } else { break; } + + // Get the parameters of the route (that is the componentProps property of the ). + const routeEntryParams = new Set(routeEntry.params ? Object.keys(routeEntry.params) : []); + // Add the parameter segments from the url. + for (const segment of routeEntry.segments) { + if (segment[0] === ':') { + routeEntryParams.add(segment.substring(1)); + } + } + + // The parameters should be the same for RouteID and RouteEntry. if (routeId.params) { const routeIdParams = Object.keys(routeId.params); - // Only compare routes with the chain that have the same number of parameters. - if (routeIdParams.length === routeChain.segments.length) { - // Maps the route's params into a path based on the path variable names, - // to compare against the route chain format. - // - // Before: - // ```ts - // { - // params: { - // s1: 'a', - // s2: 'b' - // } - // } - // ``` - // - // After: - // ```ts - // [':s1',':s2'] - // ``` - // - const pathWithParams = routeIdParams.map(key => `:${key}`); - for (let j = 0; j < pathWithParams.length; j++) { - // Skip results where the path variable is not a match - if (pathWithParams[j].toLowerCase() !== routeChain.segments[j]) { - break; - } - // Weight path matches for the same index higher. + if (routeIdParams.length === routeEntryParams.size) { + const paramsMatch = routeIdParams.every(p => routeEntryParams.has(p)); + if (paramsMatch) { score++; } - } } - // Weight id matches - score++; } + return score; }