From 56f6f56c6665f40ea6bf41be463cd416883359f7 Mon Sep 17 00:00:00 2001 From: Victor Berchet Date: Thu, 20 May 2021 07:05:49 -0700 Subject: [PATCH] fix(router): guards are now triggered on initial navigation (#23123) resolves #22936 --- core/src/components.d.ts | 4 +- core/src/components/router/readme.md | 8 +-- core/src/components/router/router.tsx | 68 ++++++++++--------- core/src/components/router/test/e2e.spec.tsx | 2 +- .../router/test/guards/basic.e2e.ts | 16 +++++ .../components/router/test/guards/index.html | 24 +++++++ .../router/test/guards/router-link.e2e.ts | 1 - .../router/test/guards/router-push.e2e.ts | 1 - .../components/router/test/matching.spec.tsx | 2 +- core/src/components/router/test/path.spec.tsx | 33 +++++---- core/src/components/router/utils/parser.ts | 6 +- core/src/components/router/utils/path.ts | 40 +++++++---- 12 files changed, 132 insertions(+), 73 deletions(-) create mode 100644 core/src/components/router/test/guards/basic.e2e.ts diff --git a/core/src/components.d.ts b/core/src/components.d.ts index 69feb020ca..f9f06e39f4 100644 --- a/core/src/components.d.ts +++ b/core/src/components.d.ts @@ -1914,7 +1914,7 @@ export namespace Components { */ "root": string; /** - * The router can work in two "modes": - With hash: `/index.html#/path/to/page` - Without hash: `/path/to/page` Using one or another might depend in the requirements of your app and/or where it's deployed. Usually "hash-less" navigation works better for SEO and it's more user friendly too, but it might requires additional server-side configuration in order to properly work. On the otherside hash-navigation is much easier to deploy, it even works over the file protocol. By default, this property is `true`, change to `false` to allow hash-less URLs. + * The router can work in two "modes": - With hash: `/index.html#/path/to/page` - Without hash: `/path/to/page` Using one or another might depend in the requirements of your app and/or where it's deployed. Usually "hash-less" navigation works better for SEO and it's more user friendly too, but it might requires additional server-side configuration in order to properly work. On the other side hash-navigation is much easier to deploy, it even works over the file protocol. By default, this property is `true`, change to `false` to allow hash-less URLs. */ "useHash": boolean; } @@ -5200,7 +5200,7 @@ declare namespace LocalJSX { */ "root"?: string; /** - * The router can work in two "modes": - With hash: `/index.html#/path/to/page` - Without hash: `/path/to/page` Using one or another might depend in the requirements of your app and/or where it's deployed. Usually "hash-less" navigation works better for SEO and it's more user friendly too, but it might requires additional server-side configuration in order to properly work. On the otherside hash-navigation is much easier to deploy, it even works over the file protocol. By default, this property is `true`, change to `false` to allow hash-less URLs. + * The router can work in two "modes": - With hash: `/index.html#/path/to/page` - Without hash: `/path/to/page` Using one or another might depend in the requirements of your app and/or where it's deployed. Usually "hash-less" navigation works better for SEO and it's more user friendly too, but it might requires additional server-side configuration in order to properly work. On the other side hash-navigation is much easier to deploy, it even works over the file protocol. By default, this property is `true`, change to `false` to allow hash-less URLs. */ "useHash"?: boolean; } diff --git a/core/src/components/router/readme.md b/core/src/components/router/readme.md index c79a5606fe..b48b808c8b 100644 --- a/core/src/components/router/readme.md +++ b/core/src/components/router/readme.md @@ -52,10 +52,10 @@ In order to configure this relationship between components (to load/select) and ## Properties -| Property | Attribute | Description | Type | Default | -| --------- | ---------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------- | ------- | -| `root` | `root` | By default `ion-router` will match the routes at the root path ("/"). That can be changed when | `string` | `'/'` | -| `useHash` | `use-hash` | The router can work in two "modes": - With hash: `/index.html#/path/to/page` - Without hash: `/path/to/page` Using one or another might depend in the requirements of your app and/or where it's deployed. Usually "hash-less" navigation works better for SEO and it's more user friendly too, but it might requires additional server-side configuration in order to properly work. On the otherside hash-navigation is much easier to deploy, it even works over the file protocol. By default, this property is `true`, change to `false` to allow hash-less URLs. | `boolean` | `true` | +| Property | Attribute | Description | Type | Default | +| --------- | ---------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------- | ------- | +| `root` | `root` | By default `ion-router` will match the routes at the root path ("/"). That can be changed when | `string` | `'/'` | +| `useHash` | `use-hash` | The router can work in two "modes": - With hash: `/index.html#/path/to/page` - Without hash: `/path/to/page` Using one or another might depend in the requirements of your app and/or where it's deployed. Usually "hash-less" navigation works better for SEO and it's more user friendly too, but it might requires additional server-side configuration in order to properly work. On the other side hash-navigation is much easier to deploy, it even works over the file protocol. By default, this property is `true`, change to `false` to allow hash-less URLs. | `boolean` | `true` | ## Events diff --git a/core/src/components/router/router.tsx b/core/src/components/router/router.tsx index 7512b56821..28401b1df3 100644 --- a/core/src/components/router/router.tsx +++ b/core/src/components/router/router.tsx @@ -40,7 +40,7 @@ export class Router implements ComponentInterface { * Usually "hash-less" navigation works better for SEO and it's more user friendly too, but it might * requires additional server-side configuration in order to properly work. * - * On the otherside hash-navigation is much easier to deploy, it even works over the file protocol. + * On the other side hash-navigation is much easier to deploy, it even works over the file protocol. * * By default, this property is `true`, change to `false` to allow hash-less URLs. */ @@ -57,11 +57,19 @@ export class Router implements ComponentInterface { @Event() ionRouteDidChange!: EventEmitter; async componentWillLoad() { - console.debug('[ion-router] router will load'); await waitUntilNavNode(); - console.debug('[ion-router] found nav'); - await this.onRoutesChanged(); + const canProceed = await this.runGuards(this.getPath()); + if (canProceed !== true) { + if (typeof canProceed === 'object') { + const { redirect } = canProceed; + const path = parsePath(redirect); + this.setPath(path.segments, ROUTER_INTENT_NONE, path.queryString); + await this.writeNavStateRoot(path.segments, ROUTER_INTENT_NONE); + } + } else { + await this.onRoutesChanged(); + } } componentDidLoad() { @@ -72,17 +80,17 @@ export class Router implements ComponentInterface { @Listen('popstate', { target: 'window' }) protected async onPopState() { const direction = this.historyDirection(); - let path = this.getPath(); + let segments = this.getPath(); - const canProceed = await this.runGuards(path); + const canProceed = await this.runGuards(segments); if (canProceed !== true) { if (typeof canProceed === 'object') { - path = parsePath(canProceed.redirect); + segments = parsePath(canProceed.redirect).segments; + } else { + return false; } - return false; } - console.debug('[ion-router] URL changed -> update nav', path, direction); - return this.writeNavStateRoot(path, direction); + return this.writeNavStateRoot(segments, direction); } @Listen('ionBackButton', { target: 'document' }) @@ -119,23 +127,20 @@ export class Router implements ComponentInterface { if (url.startsWith('.')) { url = (new URL(url, window.location.href)).pathname; } - console.debug('[ion-router] URL pushed -> updating nav', url, direction); - let path = parsePath(url); - let queryString = url.split('?')[1]; + let parsedPath = parsePath(url); - const canProceed = await this.runGuards(path); + const canProceed = await this.runGuards(parsedPath.segments); if (canProceed !== true) { if (typeof canProceed === 'object') { - path = parsePath(canProceed.redirect); - queryString = canProceed.redirect.split('?')[1]; + parsedPath = parsePath(canProceed.redirect) } else { return false; } } - this.setPath(path, direction, queryString); - return this.writeNavStateRoot(path, direction, animation); + this.setPath(parsedPath.segments, direction, parsedPath.queryString); + return this.writeNavStateRoot(parsedPath.segments, direction, animation); } /** @@ -150,8 +155,6 @@ export class Router implements ComponentInterface { /** @internal */ @Method() async printDebug() { - console.debug('CURRENT PATH', this.getPath()); - console.debug('PREVIOUS PATH', this.previousPath); printRoutes(readRoutes(this.el)); printRedirects(readRedirects(this.el)); } @@ -177,7 +180,6 @@ export class Router implements ComponentInterface { return false; } - console.debug('[ion-router] nav changed -> update URL', ids, path); this.setPath(path, direction); await this.safeWriteNavState(outlet, chain, ROUTER_INTENT_NONE, path, null, ids.length); @@ -272,24 +274,30 @@ export class Router implements ComponentInterface { } return resolve; } - private async runGuards(to: string[] | null = this.getPath(), from: string[] | null = parsePath(this.previousPath)) { + + // Executes the beforeLeave hook of the source route and the beforeEnter hook of the target route if they exist. + // + // When the beforeLeave hook does not return true (to allow navigating) then that value is returned early and the beforeEnter is executed. + // Otherwise the beforeEnterHook hook of the target route is executed. + private async runGuards(to: string[] | null = this.getPath(), from?: string[] | null) { + if (from === undefined) { + from = parsePath(this.previousPath).segments; + } + if (!to || !from) { return true; } const routes = readRoutes(this.el); - const toChain = routerPathToChain(to, routes); const fromChain = routerPathToChain(from, routes); - - const beforeEnterHook = toChain && toChain[toChain.length - 1].beforeEnter; const beforeLeaveHook = fromChain && fromChain[fromChain.length - 1].beforeLeave; const canLeave = beforeLeaveHook ? await beforeLeaveHook() : true; if (canLeave === false || typeof canLeave === 'object') { return canLeave; } - const canEnter = beforeEnterHook ? await beforeEnterHook() : true; - if (canEnter === false || typeof canEnter === 'object') { return canEnter; } + const toChain = routerPathToChain(to, routes); + const beforeEnterHook = toChain && toChain[toChain.length - 1].beforeEnter; - return true; + return beforeEnterHook ? beforeEnterHook() : true; } private async writeNavState( @@ -312,10 +320,6 @@ export class Router implements ComponentInterface { const changed = await writeNavState(node, chain, direction, index, false, animation); this.busy = false; - if (changed) { - console.debug('[ion-router] route changed', path); - } - // emit did change if (routeEvent) { this.ionRouteDidChange.emit(routeEvent); diff --git a/core/src/components/router/test/e2e.spec.tsx b/core/src/components/router/test/e2e.spec.tsx index 89692fd9f7..9edc1a6bde 100644 --- a/core/src/components/router/test/e2e.spec.tsx +++ b/core/src/components/router/test/e2e.spec.tsx @@ -77,7 +77,7 @@ describe('ionic-conference-app', () => { }); export function getRouteIDs(path: string, routes: RouteChain[]): string[] { - return routerPathToChain(parsePath(path), routes)!.map(r => r.id); + return routerPathToChain(parsePath(path).segments, routes)!.map(r => r.id); } export function getRoutePath(ids: RouteID[], routes: RouteChain[]): string { diff --git a/core/src/components/router/test/guards/basic.e2e.ts b/core/src/components/router/test/guards/basic.e2e.ts new file mode 100644 index 0000000000..e7c6d6a60c --- /dev/null +++ b/core/src/components/router/test/guards/basic.e2e.ts @@ -0,0 +1,16 @@ +import { newE2EPage } from '@stencil/core/testing'; + +test('router: guards - guards should be run on initial load', async () => { + const page = await newE2EPage({ + url: '/src/components/router/test/guards#/guard-initial-page?ionic:_testing=true' + }); + + await page.waitForChanges(); + + await checkUrl(page, '#/child/1'); +}); + +const checkUrl = async (page, url: string) => { + const getUrl = await page.url(); + expect(getUrl).toContain(url); +} \ No newline at end of file diff --git a/core/src/components/router/test/guards/index.html b/core/src/components/router/test/guards/index.html index f6775a08ac..5476beab5c 100644 --- a/core/src/components/router/test/guards/index.html +++ b/core/src/components/router/test/guards/index.html @@ -80,9 +80,26 @@ } } + class GuardInitialPage extends HTMLElement { + connectedCallback() { + this.innerHTML = ` + + + + + + Guard Initial Page + + + + `; + } + } + customElements.define('home-page', HomePage); customElements.define('child-page', ChildPage); customElements.define('test-page', TestPage); + customElements.define('guard-initial-page', GuardInitialPage); @@ -146,6 +163,7 @@ + @@ -188,6 +206,12 @@ const page = document.querySelector('ion-route[component="child-page"]'); page.beforeEnter = allow; page.beforeLeave = allow; + + + const guardPage = document.querySelector('ion-route[component="guard-initial-page"]'); + guardPage.beforeEnter = () => { + return { redirect: '/child/1' }; + } diff --git a/core/src/components/router/test/guards/router-link.e2e.ts b/core/src/components/router/test/guards/router-link.e2e.ts index c42ee0aab5..d084ee5a44 100644 --- a/core/src/components/router/test/guards/router-link.e2e.ts +++ b/core/src/components/router/test/guards/router-link.e2e.ts @@ -88,7 +88,6 @@ test('router: guards - router-link - allow/block', async () => { await checkUrl(page, '#/child/1'); }); -// TODO this is an actual bug in the code. test('router: guards - router-link - allow/redirect', async () => { const page = await newE2EPage({ url: '/src/components/router/test/guards?ionic:_testing=true' diff --git a/core/src/components/router/test/guards/router-push.e2e.ts b/core/src/components/router/test/guards/router-push.e2e.ts index 2e65ba8374..8fc057511b 100644 --- a/core/src/components/router/test/guards/router-push.e2e.ts +++ b/core/src/components/router/test/guards/router-push.e2e.ts @@ -88,7 +88,6 @@ test('router: guards - router.push - allow/block', async () => { await checkUrl(page, '#/child'); }); -// TODO this is an actual bug in the code. test('router: guards - router.push - allow/redirect', async () => { const page = await newE2EPage({ url: '/src/components/router/test/guards?ionic:_testing=true' diff --git a/core/src/components/router/test/matching.spec.tsx b/core/src/components/router/test/matching.spec.tsx index f13c703d56..41306df4b2 100644 --- a/core/src/components/router/test/matching.spec.tsx +++ b/core/src/components/router/test/matching.spec.tsx @@ -107,7 +107,7 @@ describe('matchesPath', () => { { id: '5', path: ['image'], params: { size: 'lg' } }, { id: '5', path: ['image', ':size', ':type'], params: { size: 'mg' } }, ]; - const matched = matchesPath(parsePath('/profile/manu/image/image/large/retina'), chain); + const matched = matchesPath(parsePath('/profile/manu/image/image/large/retina').segments, chain); expect(matched).toEqual([ { id: '5', path: ['profile', ':name'], params: { name: 'manu' } }, { id: '5', path: [''], params: undefined }, diff --git a/core/src/components/router/test/path.spec.tsx b/core/src/components/router/test/path.spec.tsx index 310b5b7c01..b52e5b96c4 100644 --- a/core/src/components/router/test/path.spec.tsx +++ b/core/src/components/router/test/path.spec.tsx @@ -4,43 +4,50 @@ import { chainToPath, generatePath, parsePath, readPath, writePath } from '../ut describe('parseURL', () => { it('should parse empty path', () => { - expect(parsePath('')).toEqual(['']); + expect(parsePath('').segments).toEqual(['']); }); it('should parse slash path', () => { - expect(parsePath('/')).toEqual(['']); - expect(parsePath(' / ')).toEqual(['']); + expect(parsePath('/').segments).toEqual(['']); + expect(parsePath(' / ').segments).toEqual(['']); }); it('should parse empty path (2)', () => { - expect(parsePath(' ')).toEqual(['']); + expect(parsePath(' ').segments).toEqual(['']); }); it('should parse null path', () => { - expect(parsePath(null)).toEqual(['']); + expect(parsePath(null).segments).toEqual(['']); }); it('should parse undefined path', () => { - expect(parsePath(undefined)).toEqual(['']); + expect(parsePath(undefined).segments).toEqual(['']); }); it('should parse single segment', () => { - expect(parsePath('path')).toEqual(['path']); - expect(parsePath('path/')).toEqual(['path']); - expect(parsePath('/path/')).toEqual(['path']); - expect(parsePath('/path')).toEqual(['path']); + expect(parsePath('path').segments).toEqual(['path']); + expect(parsePath('path/').segments).toEqual(['path']); + expect(parsePath('/path/').segments).toEqual(['path']); + expect(parsePath('/path').segments).toEqual(['path']); }); it('should parse relative path', () => { - expect(parsePath('path/to/file.js')).toEqual(['path', 'to', 'file.js']); + expect(parsePath('path/to/file.js').segments).toEqual(['path', 'to', 'file.js']); }); it('should parse absolute path', () => { - expect(parsePath('/path/to/file.js')).toEqual(['path', 'to', 'file.js']); + expect(parsePath('/path/to/file.js').segments).toEqual(['path', 'to', 'file.js']); }); it('should parse relative path', () => { - expect(parsePath('/PATH///to//file.js//')).toEqual(['PATH', 'to', 'file.js']); + expect(parsePath('/PATH///to//file.js//').segments).toEqual(['PATH', 'to', 'file.js']); }); + + it('should parse query string', () => { + expect(parsePath(null).queryString).toBe(undefined); + expect(parsePath('path/to/file.js').queryString).toBe(undefined); + expect(parsePath('path/to/file.js?').queryString).toEqual(''); + expect(parsePath('path/to/file.js?a=b').queryString).toEqual('a=b'); + }); }); describe('generatePath', () => { diff --git a/core/src/components/router/utils/parser.ts b/core/src/components/router/utils/parser.ts index 12d6544478..08abfcd36a 100644 --- a/core/src/components/router/utils/parser.ts +++ b/core/src/components/router/utils/parser.ts @@ -7,8 +7,8 @@ export const readRedirects = (root: Element): RouteRedirect[] => { .map(el => { const to = readProp(el, 'to'); return { - from: parsePath(readProp(el, 'from')), - to: to == null ? undefined : parsePath(to), + from: parsePath(readProp(el, 'from')).segments, + to: to == null ? undefined : parsePath(to).segments, }; }); }; @@ -26,7 +26,7 @@ export const readRouteNodes = (root: Element, node = root): RouteTree => { throw new Error('component missing in ion-route'); } return { - path: parsePath(readProp(el, 'url')), + path: parsePath(readProp(el, 'url')).segments, id: component.toLowerCase(), params: el.componentProps, beforeLeave: el.beforeLeave, diff --git a/core/src/components/router/utils/path.ts b/core/src/components/router/utils/path.ts index 3a6e315f67..f005f5c78f 100644 --- a/core/src/components/router/utils/path.ts +++ b/core/src/components/router/utils/path.ts @@ -30,7 +30,7 @@ export const chainToPath = (chain: RouteChain): string[] | null => { export const writePath = (history: History, root: string, useHash: boolean, path: string[], direction: RouterDirection, state: number, queryString?: string) => { let url = generatePath([ - ...parsePath(root), + ...parsePath(root).segments, ...path ]); if (useHash) { @@ -73,23 +73,33 @@ export const readPath = (loc: Location, root: string, useHash: boolean): string[ : ''; } - const prefix = parsePath(root); - const path = parsePath(pathname); + const prefix = parsePath(root).segments; + const path = parsePath(pathname).segments; return removePrefix(prefix, path); }; -export const parsePath = (path: string | undefined | null): string[] => { - if (path == null) { - return ['']; - } - const removeQueryString = path.split('?')[0]; - const segments = removeQueryString.split('/') - .map(s => s.trim()) - .filter(s => s.length > 0); +// Parses the path to: +// - segments an array of '/' separated parts, +// - queryString (undefined when no query string). +export const parsePath = (path: string | undefined | null): {segments: string[], queryString?: string} => { + let segments = ['']; + let queryString; - if (segments.length === 0) { - return ['']; - } else { - return segments; + if (path != null) { + const qsStart = path.indexOf('?'); + if (qsStart > -1) { + queryString = path.substr(qsStart + 1); + path = path.substr(0, qsStart); + } + + segments = path.split('/') + .map(s => s.trim()) + .filter(s => s.length > 0); + + if (segments.length === 0) { + segments = ['']; + } } + + return { segments, queryString }; };