From 3a1f292b5afd0a3614c75026a0fa55ca9a1e1eb8 Mon Sep 17 00:00:00 2001 From: Ryan Christian Date: Thu, 22 Aug 2024 00:56:17 -0500 Subject: [PATCH 1/6] feat: Add `limit` prop to router, first pass --- src/router.d.ts | 6 +- src/router.js | 12 ++- test/router.test.js | 195 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 209 insertions(+), 4 deletions(-) diff --git a/src/router.d.ts b/src/router.d.ts index f658874..433e700 100644 --- a/src/router.d.ts +++ b/src/router.d.ts @@ -1,6 +1,10 @@ import { AnyComponent, FunctionComponent, VNode } from 'preact'; -export const LocationProvider: FunctionComponent; +export function LocationProvider(props: { + url?: string; + limit?: string | RegExp; + children?: VNode[]; +}): VNode; type NestedArray = Array>; diff --git a/src/router.js b/src/router.js index 354f451..a8e29cf 100644 --- a/src/router.js +++ b/src/router.js @@ -7,7 +7,7 @@ import { useContext, useMemo, useReducer, useLayoutEffect, useRef } from 'preact * @typedef {import('./internal.d.ts').VNode} VNode */ -let push; +let push, limit; const UPDATE = (state, url) => { push = undefined; if (url && url.type === 'click') { @@ -16,12 +16,17 @@ const UPDATE = (state, url) => { return state; } - const link = url.target.closest('a[href]'); + const link = url.target.closest('a[href]'), + href = link.getAttribute('href'); if ( !link || link.origin != location.origin || /^#/.test(link.getAttribute('href')) || - !/^(_?self)?$/i.test(link.target) + !/^(_?self)?$/i.test(link.target) || + limit && (typeof limit == 'string' + ? !href.startsWith(limit) + : !limit.test(href) + ) ) { return state; } @@ -72,6 +77,7 @@ export const exec = (url, route, matches) => { export function LocationProvider(props) { const [url, route] = useReducer(UPDATE, props.url || location.pathname + location.search); + if (props.limit) limit = props.limit; const wasPush = push === true; const value = useMemo(() => { diff --git a/test/router.test.js b/test/router.test.js index d4e0d1f..8f9cedd 100644 --- a/test/router.test.js +++ b/test/router.test.js @@ -583,6 +583,201 @@ describe('Router', () => { } }); + describe.skip('intercepted VS external links with `limit`', () => { + const shouldIntercept = '/app'; + const shouldNavigate = '/site'; + + // prevent actual navigations (not implemented in JSDOM) + const clickHandler = jest.fn(e => e.preventDefault()); + + const Route = jest.fn( + () => html` +
+ Internal Link + External Link +
+ ` + ); + + let pushState, loc; + + beforeAll(() => { + pushState = jest.spyOn(history, 'pushState'); + addEventListener('click', clickHandler); + }); + + afterAll(() => { + pushState.mockRestore(); + removeEventListener('click', clickHandler); + }); + + beforeEach(async () => { + Route.mockClear(); + clickHandler.mockClear(); + pushState.mockClear(); + }); + + it('should intercept clicks on links matching the `limit` props (string)', async () => { + render( + html` + <${LocationProvider} limit="/app"> + <${Router}> + <${Route} default /> + + <${() => { + loc = useLocation(); + }} /> + + `, + scratch + ); + await sleep(10); + + const el = scratch.querySelector(`a[href="${shouldIntercept}"]`); + el.click(); + await sleep(1); + expect(loc).toMatchObject({ url: shouldIntercept }); + expect(Route).toHaveBeenCalledTimes(1); + expect(pushState).toHaveBeenCalledWith(null, '', shouldIntercept); + expect(clickHandler).toHaveBeenCalled(); + }); + + it('should navigate clicks on links not matching the `limit` props (string)', async () => { + render( + html` + <${LocationProvider} limit="app"> + <${Router}> + <${Route} default /> + + <${() => { + loc = useLocation(); + }} /> + + `, + scratch + ); + await sleep(10); + + const el = scratch.querySelector(`a[href="${shouldNavigate}"]`); + el.click(); + await sleep(1); + expect(Route).not.toHaveBeenCalled(); + expect(pushState).not.toHaveBeenCalled(); + expect(clickHandler).toHaveBeenCalled(); + }); + + it('should intercept clicks on links matching the `limit` props (regex)', async () => { + render( + html` + <${LocationProvider} limit={/^\/app/}> + <${Router}> + <${Route} default /> + + <${() => { + loc = useLocation(); + }} /> + + `, + scratch + ); + await sleep(10); + + const el = scratch.querySelector(`a[href="${shouldIntercept}"]`); + el.click(); + await sleep(1); + expect(loc).toMatchObject({ url: shouldIntercept }); + expect(Route).toHaveBeenCalledTimes(1); + expect(pushState).toHaveBeenCalledWith(null, '', shouldIntercept); + expect(clickHandler).toHaveBeenCalled(); + }); + + it('should navigate clicks on links not matching the `limit` props (regex)', async () => { + render( + html` + <${LocationProvider} limit={/^\/app/}> + <${Router}> + <${Route} default /> + + <${() => { + loc = useLocation(); + }} /> + + `, + scratch + ); + await sleep(10); + + const el = scratch.querySelector(`a[href="${shouldNavigate}"]`); + el.click(); + await sleep(1); + expect(Route).not.toHaveBeenCalled(); + expect(pushState).not.toHaveBeenCalled(); + expect(clickHandler).toHaveBeenCalled(); + }); + }); + + it.skip('should ignore links excluded by `limit`', async () => { + let loc; + const pushState = jest.spyOn(history, 'pushState'); + + // prevent actual navigations (not implemented in JSDOM) + const clickHandler = jest.fn(e => e.preventDefault()); + addEventListener('click', clickHandler); + + const Route = jest.fn( + () => html` +
+ Internal Link + External Link +
+ ` + ); + + render( + html` + <${LocationProvider} limit="/app"> + <${Router}> + <${Route} default /> + + <${() => { + loc = useLocation(); + }} /> + + `, + scratch + ); + + await sleep(10); + Route.mockClear(); + clickHandler.mockClear(); + pushState.mockClear(); + + let el = scratch.querySelector('a[href="/app"]'); + el.click(); + await sleep(20); + expect(loc).toMatchObject({ url: '/app' }); + expect(Route).toHaveBeenCalledTimes(1); + expect(pushState).toHaveBeenCalledWith(null, '', '/app'); + expect(clickHandler).toHaveBeenCalled(); + + await sleep(10); + Route.mockClear(); + clickHandler.mockClear(); + pushState.mockClear(); + + el = scratch.querySelector('a[href="/site"]'); + el.click(); + await sleep(1); + expect(Route).not.toHaveBeenCalled(); + expect(pushState).not.toHaveBeenCalled(); + expect(clickHandler).toHaveBeenCalled(); + + Route.mockRestore(); + pushState.mockRestore(); + clickHandler.mockRestore(); + removeEventListener('click', clickHandler); + }); + it('should scroll to top when navigating forward', async () => { const scrollTo = sinon.spy(window, 'scrollTo'); From 143cd15dbd09d21384379fd29fbbff75cf3f84c2 Mon Sep 17 00:00:00 2001 From: Ryan Christian Date: Mon, 14 Oct 2024 18:36:59 -0500 Subject: [PATCH 2/6] test: Clean up --- src/router.d.ts | 1 - src/router.js | 2 +- test/router.test.js | 244 ++++++++++++++++++-------------------------- 3 files changed, 98 insertions(+), 149 deletions(-) diff --git a/src/router.d.ts b/src/router.d.ts index 433e700..1cdcddc 100644 --- a/src/router.d.ts +++ b/src/router.d.ts @@ -1,7 +1,6 @@ import { AnyComponent, FunctionComponent, VNode } from 'preact'; export function LocationProvider(props: { - url?: string; limit?: string | RegExp; children?: VNode[]; }): VNode; diff --git a/src/router.js b/src/router.js index a8e29cf..02c193a 100644 --- a/src/router.js +++ b/src/router.js @@ -21,7 +21,7 @@ const UPDATE = (state, url) => { if ( !link || link.origin != location.origin || - /^#/.test(link.getAttribute('href')) || + /^#/.test(href) || !/^(_?self)?$/i.test(link.target) || limit && (typeof limit == 'string' ? !href.startsWith(limit) diff --git a/test/router.test.js b/test/router.test.js index 8f9cedd..f88883c 100644 --- a/test/router.test.js +++ b/test/router.test.js @@ -505,7 +505,6 @@ describe('Router', () => { const shouldIntercept = [null, '', '_self', 'self', '_SELF']; const shouldNavigate = ['_top', '_parent', '_blank', 'custom', '_BLANK']; - // prevent actual navigations (not implemented in JSDOM) const clickHandler = sinon.fake(e => e.preventDefault()); const Route = sinon.fake( @@ -583,199 +582,150 @@ describe('Router', () => { } }); - describe.skip('intercepted VS external links with `limit`', () => { - const shouldIntercept = '/app'; - const shouldNavigate = '/site'; + describe('intercepted VS external links with `limit`', () => { + const shouldIntercept = ['/app', '/app/deeper']; + const shouldNavigate = ['/site', '/site/deeper']; - // prevent actual navigations (not implemented in JSDOM) - const clickHandler = jest.fn(e => e.preventDefault()); + const clickHandler = sinon.fake(e => e.preventDefault()); - const Route = jest.fn( - () => html` + const Route = sinon.fake( + () => (
Internal Link + Internal Deeper Link External Link + External Deeper Link
- ` + ) ); - let pushState, loc; + let pushState; - beforeAll(() => { - pushState = jest.spyOn(history, 'pushState'); + before(() => { + pushState = sinon.spy(history, 'pushState'); addEventListener('click', clickHandler); }); - afterAll(() => { - pushState.mockRestore(); + after(() => { + pushState.restore(); removeEventListener('click', clickHandler); }); beforeEach(async () => { - Route.mockClear(); - clickHandler.mockClear(); - pushState.mockClear(); + Route.resetHistory(); + clickHandler.resetHistory(); + pushState.resetHistory(); }); it('should intercept clicks on links matching the `limit` props (string)', async () => { render( - html` - <${LocationProvider} limit="/app"> - <${Router}> - <${Route} default /> - - <${() => { - loc = useLocation(); - }} /> - - `, + + + + + + , scratch ); + Route.resetHistory(); await sleep(10); - const el = scratch.querySelector(`a[href="${shouldIntercept}"]`); - el.click(); - await sleep(1); - expect(loc).toMatchObject({ url: shouldIntercept }); - expect(Route).toHaveBeenCalledTimes(1); - expect(pushState).toHaveBeenCalledWith(null, '', shouldIntercept); - expect(clickHandler).toHaveBeenCalled(); + for (const url of shouldIntercept) { + const el = scratch.querySelector(`a[href="${url}"]`); + el.click(); + await sleep(1); + expect(loc).to.deep.include({ url }); + expect(Route).to.have.been.calledOnce; + expect(pushState).to.have.been.calledWith(null, '', url); + expect(clickHandler).to.have.been.called; + + Route.resetHistory(); + pushState.resetHistory(); + clickHandler.resetHistory(); + } }); - it('should navigate clicks on links not matching the `limit` props (string)', async () => { + it('should allow default browser navigation for links not matching the `limit` props (string)', async () => { render( - html` - <${LocationProvider} limit="app"> - <${Router}> - <${Route} default /> - - <${() => { - loc = useLocation(); - }} /> - - `, + + + + + + , scratch ); + Route.resetHistory(); await sleep(10); - const el = scratch.querySelector(`a[href="${shouldNavigate}"]`); - el.click(); - await sleep(1); - expect(Route).not.toHaveBeenCalled(); - expect(pushState).not.toHaveBeenCalled(); - expect(clickHandler).toHaveBeenCalled(); + for (const url of shouldNavigate) { + const el = scratch.querySelector(`a[href="${url}"]`); + el.click(); + await sleep(1); + expect(Route).not.to.have.been.called; + expect(pushState).not.to.have.been.called; + expect(clickHandler).to.have.been.called; + + Route.resetHistory(); + pushState.resetHistory(); + clickHandler.resetHistory(); + } }); it('should intercept clicks on links matching the `limit` props (regex)', async () => { render( - html` - <${LocationProvider} limit={/^\/app/}> - <${Router}> - <${Route} default /> - - <${() => { - loc = useLocation(); - }} /> - - `, + + + + + + , scratch ); + Route.resetHistory(); await sleep(10); - const el = scratch.querySelector(`a[href="${shouldIntercept}"]`); - el.click(); - await sleep(1); - expect(loc).toMatchObject({ url: shouldIntercept }); - expect(Route).toHaveBeenCalledTimes(1); - expect(pushState).toHaveBeenCalledWith(null, '', shouldIntercept); - expect(clickHandler).toHaveBeenCalled(); + for (const url of shouldIntercept) { + const el = scratch.querySelector(`a[href="${url}"]`); + el.click(); + await sleep(1); + expect(loc).to.deep.include({ url }); + expect(Route).to.have.been.calledOnce; + expect(pushState).to.have.been.calledWith(null, '', url); + expect(clickHandler).to.have.been.called; + + Route.resetHistory(); + pushState.resetHistory(); + clickHandler.resetHistory(); + } }); - it('should navigate clicks on links not matching the `limit` props (regex)', async () => { + it('should allow default browser navigation for links not matching the `limit` props (regex)', async () => { render( - html` - <${LocationProvider} limit={/^\/app/}> - <${Router}> - <${Route} default /> - - <${() => { - loc = useLocation(); - }} /> - - `, + + + + + + , scratch ); + Route.resetHistory(); await sleep(10); - const el = scratch.querySelector(`a[href="${shouldNavigate}"]`); - el.click(); - await sleep(1); - expect(Route).not.toHaveBeenCalled(); - expect(pushState).not.toHaveBeenCalled(); - expect(clickHandler).toHaveBeenCalled(); - }); - }); - - it.skip('should ignore links excluded by `limit`', async () => { - let loc; - const pushState = jest.spyOn(history, 'pushState'); - - // prevent actual navigations (not implemented in JSDOM) - const clickHandler = jest.fn(e => e.preventDefault()); - addEventListener('click', clickHandler); - - const Route = jest.fn( - () => html` -
- Internal Link - External Link -
- ` - ); - - render( - html` - <${LocationProvider} limit="/app"> - <${Router}> - <${Route} default /> - - <${() => { - loc = useLocation(); - }} /> - - `, - scratch - ); - - await sleep(10); - Route.mockClear(); - clickHandler.mockClear(); - pushState.mockClear(); - - let el = scratch.querySelector('a[href="/app"]'); - el.click(); - await sleep(20); - expect(loc).toMatchObject({ url: '/app' }); - expect(Route).toHaveBeenCalledTimes(1); - expect(pushState).toHaveBeenCalledWith(null, '', '/app'); - expect(clickHandler).toHaveBeenCalled(); - - await sleep(10); - Route.mockClear(); - clickHandler.mockClear(); - pushState.mockClear(); + for (const url of shouldNavigate) { + const el = scratch.querySelector(`a[href="${url}"]`); + el.click(); + await sleep(1); + expect(Route).not.to.have.been.called; + expect(pushState).not.to.have.been.called; + expect(clickHandler).to.have.been.called; - el = scratch.querySelector('a[href="/site"]'); - el.click(); - await sleep(1); - expect(Route).not.toHaveBeenCalled(); - expect(pushState).not.toHaveBeenCalled(); - expect(clickHandler).toHaveBeenCalled(); - - Route.mockRestore(); - pushState.mockRestore(); - clickHandler.mockRestore(); - removeEventListener('click', clickHandler); + Route.resetHistory(); + pushState.resetHistory(); + clickHandler.resetHistory(); + } + }); }); it('should scroll to top when navigating forward', async () => { From 8b04312e4451d780d87cc96d31be7c8b604780a0 Mon Sep 17 00:00:00 2001 From: Ryan Christian Date: Mon, 14 Oct 2024 18:42:34 -0500 Subject: [PATCH 3/6] docs: Add instructions to README for `limit` prop --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index 152515d..ee8b2f3 100644 --- a/README.md +++ b/README.md @@ -82,6 +82,10 @@ export async function prerender(data) { A context provider that provides the current location to its children. This is required for the router to function. +Props: + +- `limit?: string | RegExp` - Sets a limit on the paths that the router will handle (intercept). If a path does not match the limit, either by starting with the provided string or matching the RegExp, the router will ignore it and default browser navigation will apply. + Typically, you would wrap your entire app in this provider: ```js From b8092aff93a3435064fb53d95023c61bf7f7fa08 Mon Sep 17 00:00:00 2001 From: Ryan Christian Date: Wed, 16 Oct 2024 17:30:10 -0500 Subject: [PATCH 4/6] refactor: Switch to `scope` --- README.md | 4 ++-- src/router.d.ts | 2 +- src/router.js | 14 +++++++++----- test/router.test.js | 12 ++++++------ 4 files changed, 18 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index ee8b2f3..8a9a7bc 100644 --- a/README.md +++ b/README.md @@ -84,7 +84,7 @@ A context provider that provides the current location to its children. This is r Props: -- `limit?: string | RegExp` - Sets a limit on the paths that the router will handle (intercept). If a path does not match the limit, either by starting with the provided string or matching the RegExp, the router will ignore it and default browser navigation will apply. +- `scope?: string | RegExp` - Sets a scope for the paths that the router will handle (intercept). If a path does not match the scope, either by starting with the provided string or matching the RegExp, the router will ignore it and default browser navigation will apply. Typically, you would wrap your entire app in this provider: @@ -92,7 +92,7 @@ Typically, you would wrap your entire app in this provider: import { LocationProvider } from 'preact-iso'; const App = () => ( - + {/* Your app here */} ); diff --git a/src/router.d.ts b/src/router.d.ts index 1cdcddc..9530923 100644 --- a/src/router.d.ts +++ b/src/router.d.ts @@ -1,7 +1,7 @@ import { AnyComponent, FunctionComponent, VNode } from 'preact'; export function LocationProvider(props: { - limit?: string | RegExp; + scope?: string | RegExp; children?: VNode[]; }): VNode; diff --git a/src/router.js b/src/router.js index 02c193a..b999cce 100644 --- a/src/router.js +++ b/src/router.js @@ -7,7 +7,7 @@ import { useContext, useMemo, useReducer, useLayoutEffect, useRef } from 'preact * @typedef {import('./internal.d.ts').VNode} VNode */ -let push, limit; +let push, scope; const UPDATE = (state, url) => { push = undefined; if (url && url.type === 'click') { @@ -23,9 +23,9 @@ const UPDATE = (state, url) => { link.origin != location.origin || /^#/.test(href) || !/^(_?self)?$/i.test(link.target) || - limit && (typeof limit == 'string' - ? !href.startsWith(limit) - : !limit.test(href) + scope && (typeof scope == 'string' + ? !href.startsWith(scope) + : !scope.test(href) ) ) { return state; @@ -75,9 +75,13 @@ export const exec = (url, route, matches) => { return matches; }; +/** + * @type {import('./router.d.ts').LocationProvider} + */ export function LocationProvider(props) { + // @ts-expect-error - props.url is not implemented correctly & will be removed in the future const [url, route] = useReducer(UPDATE, props.url || location.pathname + location.search); - if (props.limit) limit = props.limit; + if (props.scope) scope = props.scope; const wasPush = push === true; const value = useMemo(() => { diff --git a/test/router.test.js b/test/router.test.js index f88883c..019edc9 100644 --- a/test/router.test.js +++ b/test/router.test.js @@ -582,7 +582,7 @@ describe('Router', () => { } }); - describe('intercepted VS external links with `limit`', () => { + describe('intercepted VS external links with `scope`', () => { const shouldIntercept = ['/app', '/app/deeper']; const shouldNavigate = ['/site', '/site/deeper']; @@ -617,9 +617,9 @@ describe('Router', () => { pushState.resetHistory(); }); - it('should intercept clicks on links matching the `limit` props (string)', async () => { + it('should intercept clicks on links matching the `scope` props (string)', async () => { render( - + @@ -647,7 +647,7 @@ describe('Router', () => { it('should allow default browser navigation for links not matching the `limit` props (string)', async () => { render( - + @@ -674,7 +674,7 @@ describe('Router', () => { it('should intercept clicks on links matching the `limit` props (regex)', async () => { render( - + @@ -702,7 +702,7 @@ describe('Router', () => { it('should allow default browser navigation for links not matching the `limit` props (regex)', async () => { render( - + From 86a173eda9962d182c95f9a97302a1a34a9dc8b5 Mon Sep 17 00:00:00 2001 From: Ryan Christian Date: Wed, 16 Oct 2024 17:33:58 -0500 Subject: [PATCH 5/6] test: Simplify tests a bit --- test/router.test.js | 45 +++++++++++---------------------------------- 1 file changed, 11 insertions(+), 34 deletions(-) diff --git a/test/router.test.js b/test/router.test.js index 019edc9..4450dd8 100644 --- a/test/router.test.js +++ b/test/router.test.js @@ -588,15 +588,13 @@ describe('Router', () => { const clickHandler = sinon.fake(e => e.preventDefault()); - const Route = sinon.fake( - () => ( - - ) + const Links = () => ( + <> + Internal Link + Internal Deeper Link + External Link + External Deeper Link + ); let pushState; @@ -612,7 +610,6 @@ describe('Router', () => { }); beforeEach(async () => { - Route.resetHistory(); clickHandler.resetHistory(); pushState.resetHistory(); }); @@ -620,14 +617,11 @@ describe('Router', () => { it('should intercept clicks on links matching the `scope` props (string)', async () => { render( - - - + , scratch ); - Route.resetHistory(); await sleep(10); for (const url of shouldIntercept) { @@ -635,11 +629,9 @@ describe('Router', () => { el.click(); await sleep(1); expect(loc).to.deep.include({ url }); - expect(Route).to.have.been.calledOnce; expect(pushState).to.have.been.calledWith(null, '', url); expect(clickHandler).to.have.been.called; - Route.resetHistory(); pushState.resetHistory(); clickHandler.resetHistory(); } @@ -648,25 +640,20 @@ describe('Router', () => { it('should allow default browser navigation for links not matching the `limit` props (string)', async () => { render( - - - + , scratch ); - Route.resetHistory(); await sleep(10); for (const url of shouldNavigate) { const el = scratch.querySelector(`a[href="${url}"]`); el.click(); await sleep(1); - expect(Route).not.to.have.been.called; expect(pushState).not.to.have.been.called; expect(clickHandler).to.have.been.called; - Route.resetHistory(); pushState.resetHistory(); clickHandler.resetHistory(); } @@ -675,14 +662,11 @@ describe('Router', () => { it('should intercept clicks on links matching the `limit` props (regex)', async () => { render( - - - + , scratch ); - Route.resetHistory(); await sleep(10); for (const url of shouldIntercept) { @@ -690,11 +674,9 @@ describe('Router', () => { el.click(); await sleep(1); expect(loc).to.deep.include({ url }); - expect(Route).to.have.been.calledOnce; expect(pushState).to.have.been.calledWith(null, '', url); expect(clickHandler).to.have.been.called; - Route.resetHistory(); pushState.resetHistory(); clickHandler.resetHistory(); } @@ -703,25 +685,20 @@ describe('Router', () => { it('should allow default browser navigation for links not matching the `limit` props (regex)', async () => { render( - - - + , scratch ); - Route.resetHistory(); await sleep(10); for (const url of shouldNavigate) { const el = scratch.querySelector(`a[href="${url}"]`); el.click(); await sleep(1); - expect(Route).not.to.have.been.called; expect(pushState).not.to.have.been.called; expect(clickHandler).to.have.been.called; - Route.resetHistory(); pushState.resetHistory(); clickHandler.resetHistory(); } From c9e5a20673e550189e01956c7b86cf7d8cb60621 Mon Sep 17 00:00:00 2001 From: Ryan Christian Date: Wed, 16 Oct 2024 19:37:48 -0500 Subject: [PATCH 6/6] fix: LocationProvider type to support single child --- src/router.d.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/router.d.ts b/src/router.d.ts index 9530923..7958546 100644 --- a/src/router.d.ts +++ b/src/router.d.ts @@ -1,8 +1,8 @@ -import { AnyComponent, FunctionComponent, VNode } from 'preact'; +import { AnyComponent, ComponentChildren, FunctionComponent, VNode } from 'preact'; export function LocationProvider(props: { scope?: string | RegExp; - children?: VNode[]; + children?: ComponentChildren; }): VNode; type NestedArray = Array>;