From 46d57141ddc99006a5cbf4ac96987a01dc158b42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ramirez=20Vargas=2C=20Jos=C3=A9=20Pablo?= Date: Sun, 7 Sep 2025 01:03:37 -0600 Subject: [PATCH 1/3] fix: Ensure LocationFull validates state data before relaying it to history API --- src/lib/core/LocationFull.svelte.test.ts | 49 +++++++++++++++++++----- src/lib/core/LocationFull.ts | 7 +++- 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/src/lib/core/LocationFull.svelte.test.ts b/src/lib/core/LocationFull.svelte.test.ts index 38334e8..6c2546e 100644 --- a/src/lib/core/LocationFull.svelte.test.ts +++ b/src/lib/core/LocationFull.svelte.test.ts @@ -1,7 +1,6 @@ import { afterEach, beforeAll, beforeEach, describe, expect, test, vi } from "vitest"; import { LocationFull } from "./LocationFull.js"; import type { State, Location } from "$lib/types.js"; -import { flushSync } from "svelte"; import { joinPaths } from "./RouterEngine.svelte.js"; describe("LocationFull", () => { @@ -62,7 +61,6 @@ describe("LocationFull", () => { // Act. globalThis.window.history.pushState(null, '', 'http://example.com/other'); - flushSync(); // Assert. expect(callback).toHaveBeenCalledOnce(); @@ -79,13 +77,12 @@ describe("LocationFull", () => { ])("Should provide the URL, state and method $method via the event object of 'beforeNavigate'.", ({ method, stateFn }) => { // Arrange. const callback = vi.fn(); - const state = { test: 'value' }; + const state = { path: { test: 'value' }, hash: {} }; location.on('beforeNavigate', callback); // Act. // @ts-expect-error stateFn cannot enumerate history. globalThis.window.history[stateFn](state, '', 'http://example.com/other'); - flushSync(); // Assert. expect(callback).toHaveBeenCalledWith({ @@ -105,7 +102,6 @@ describe("LocationFull", () => { // Act. globalThis.window.history.pushState(null, '', 'http://example.com/other'); - flushSync(); // Assert. expect(callback).toHaveBeenCalledWith({ @@ -126,7 +122,6 @@ describe("LocationFull", () => { // Act. globalThis.window.history.pushState(null, '', 'http://example.com/other'); - flushSync(); // Assert. expect(callback).toHaveBeenCalledWith({ @@ -138,6 +133,27 @@ describe("LocationFull", () => { cancel: expect.any(Function) }); }); + test.each([ + 'pushState' as const, + 'replaceState' as const, + ])("Should ultimately push the state data via the %s method set by beforeNavigate handlers in event.state.", (stateFn) => { + // Arrange. + const state = { path: { test: 'value' }, hash: {} }; + const callback = vi.fn((event) => { + event.state = state; + }); + const unSub = location.on('beforeNavigate', callback); + + // Act. + globalThis.window.history[stateFn](null, '', 'http://example.com/other'); + + // Assert. + expect(callback).toHaveBeenCalledOnce(); + expect(globalThis.window.history.state).deep.equal(state); + + // Cleanup. + unSub(); + }); test("Should register the provided callback for the 'navigationCancelled' event.", () => { // Arrange. const callback = vi.fn(); @@ -146,7 +162,6 @@ describe("LocationFull", () => { // Act. globalThis.window.history.pushState(null, '', 'http://example.com/other'); - flushSync(); // Assert. expect(callback).toHaveBeenCalledOnce(); @@ -161,7 +176,6 @@ describe("LocationFull", () => { // Act. globalThis.window.history.pushState(state, '', 'http://example.com/other'); - flushSync(); // Assert. expect(callback).toHaveBeenCalledWith({ url: 'http://example.com/other', cause: 'test', method: 'push', state }); @@ -177,7 +191,6 @@ describe("LocationFull", () => { // Act. globalThis.window.history[fn](null, '', newUrl); - flushSync(); // Assert. expect(location.url.href).toBe(newUrl); @@ -193,7 +206,6 @@ describe("LocationFull", () => { // Act. globalThis.window.history[fn](state, '', 'http://example.com/new'); - flushSync(); // Assert. expect(location.getState(false)).toEqual(state.path); @@ -201,4 +213,21 @@ describe("LocationFull", () => { expect(location.getState('p1')).toEqual(state.hash.p1); }); }); + describe('Navigation Interception', () => { + test.each([ + 'pushState' as const, + 'replaceState' as const, + ])("Should preserve the previous valid state whenever %s is called with non-conformant state.", (stateFn) => { + // Arrange. + const validState = { path: { test: 'value' }, hash: {} }; + globalThis.window.history[stateFn](validState, '', 'http://example.com/'); + const state = { test: 'value' }; + + // Act. + globalThis.window.history[stateFn](state, '', 'http://example.com/other'); + + // Assert. + expect(globalThis.window.history.state).deep.equals(validState); + }); + }); }); \ No newline at end of file diff --git a/src/lib/core/LocationFull.ts b/src/lib/core/LocationFull.ts index 3dda21a..481629d 100644 --- a/src/lib/core/LocationFull.ts +++ b/src/lib/core/LocationFull.ts @@ -1,4 +1,5 @@ import type { BeforeNavigateEvent, Events, NavigationCancelledEvent, NavigationEvent } from "$lib/types.js"; +import { isConformantState } from "./isConformantState.js"; import { LocationLite } from "./LocationLite.svelte.js"; import { LocationState } from "./LocationState.svelte.js"; @@ -51,12 +52,16 @@ export class LocationFull extends LocationLite { for (let sub of Object.values(this.#eventSubs.navigationCancelled)) { sub({ url, - state, + state: event.state, method, cause: event.cancelReason, }); } } else { + if (!isConformantState(event.state)) { + console.warn("Warning: Non-conformant state object passed to history." + method + "State. Previous state will prevail."); + event.state = this.#innerState.state; + } const navFn = method === 'push' ? this.#originalPushState : this.#originalReplaceState; navFn(event.state, '', url); this.url.href = globalThis.window?.location.href; From 711d94cec40eee5a3ddbc1c8b04c3f49de0f5a35 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ramirez=20Vargas=2C=20Jos=C3=A9=20Pablo?= Date: Sun, 7 Sep 2025 01:11:50 -0600 Subject: [PATCH 2/3] chore(tests): Rename LocationFull's test file and do proper cleanup --- ...ll.svelte.test.ts => LocationFull.test.ts} | 45 ++++++++++++++----- 1 file changed, 34 insertions(+), 11 deletions(-) rename src/lib/core/{LocationFull.svelte.test.ts => LocationFull.test.ts} (86%) diff --git a/src/lib/core/LocationFull.svelte.test.ts b/src/lib/core/LocationFull.test.ts similarity index 86% rename from src/lib/core/LocationFull.svelte.test.ts rename to src/lib/core/LocationFull.test.ts index 6c2546e..c16ad41 100644 --- a/src/lib/core/LocationFull.svelte.test.ts +++ b/src/lib/core/LocationFull.test.ts @@ -57,13 +57,16 @@ describe("LocationFull", () => { test("Should register the provided callback for the 'beforeNavigate' event.", () => { // Arrange. const callback = vi.fn(); - location.on('beforeNavigate', callback); + const unSub = location.on('beforeNavigate', callback); // Act. globalThis.window.history.pushState(null, '', 'http://example.com/other'); // Assert. expect(callback).toHaveBeenCalledOnce(); + + // Cleanup. + unSub(); }); test.each([ { @@ -78,7 +81,7 @@ describe("LocationFull", () => { // Arrange. const callback = vi.fn(); const state = { path: { test: 'value' }, hash: {} }; - location.on('beforeNavigate', callback); + const unSub = location.on('beforeNavigate', callback); // Act. // @ts-expect-error stateFn cannot enumerate history. @@ -93,12 +96,15 @@ describe("LocationFull", () => { cancelReason: undefined, cancel: expect.any(Function) }); + + // Cleanup. + unSub(); }); test("Should set wasCancelled to true and cancelReason to the provided reason when the event is cancelled to subsequent callbacks.", () => { // Arrange. const callback = vi.fn(); - location.on('beforeNavigate', (event) => event.cancel('test')); - location.on('beforeNavigate', callback); + const unSub1 = location.on('beforeNavigate', (event) => event.cancel('test')); + const unSub2 = location.on('beforeNavigate', callback); // Act. globalThis.window.history.pushState(null, '', 'http://example.com/other'); @@ -112,13 +118,17 @@ describe("LocationFull", () => { cancelReason: 'test', cancel: expect.any(Function) }); + + // Cleanup. + unSub1(); + unSub2(); }); test("Should ignore cancellation reasons from callbacks if the event has already been cancelled.", () => { // Arrange. const callback = vi.fn(); - location.on('beforeNavigate', (event) => event.cancel('test')); - location.on('beforeNavigate', (event) => event.cancel('ignored')); - location.on('beforeNavigate', callback); + const unSub1 = location.on('beforeNavigate', (event) => event.cancel('test')); + const unSub2 = location.on('beforeNavigate', (event) => event.cancel('ignored')); + const unSub3 = location.on('beforeNavigate', callback); // Act. globalThis.window.history.pushState(null, '', 'http://example.com/other'); @@ -132,6 +142,11 @@ describe("LocationFull", () => { cancelReason: 'test', cancel: expect.any(Function) }); + + // Cleanup. + unSub1(); + unSub2(); + unSub3(); }); test.each([ 'pushState' as const, @@ -157,28 +172,36 @@ describe("LocationFull", () => { test("Should register the provided callback for the 'navigationCancelled' event.", () => { // Arrange. const callback = vi.fn(); - location.on('beforeNavigate', (event) => event.cancel()); - location.on('navigationCancelled', callback); + const unSub1 = location.on('beforeNavigate', (event) => event.cancel()); + const unSub2 = location.on('navigationCancelled', callback); // Act. globalThis.window.history.pushState(null, '', 'http://example.com/other'); // Assert. expect(callback).toHaveBeenCalledOnce(); + + // Cleanup. + unSub1(); + unSub2(); }); test("Should transfer the cause of cancellation and the state to the 'navigationCancelled' event.", () => { // Arrange. const callback = vi.fn(); const reason = 'test'; const state = { test: 'value' }; - location.on('beforeNavigate', (event) => event.cancel(reason)); - location.on('navigationCancelled', callback); + const unSub1 = location.on('beforeNavigate', (event) => event.cancel(reason)); + const unSub2 = location.on('navigationCancelled', callback); // Act. globalThis.window.history.pushState(state, '', 'http://example.com/other'); // Assert. expect(callback).toHaveBeenCalledWith({ url: 'http://example.com/other', cause: 'test', method: 'push', state }); + + // Cleanup. + unSub1(); + unSub2(); }); }); describe('url', () => { From 7df9b33c016a908af8eb29549d70b4a88c6c6d0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ramirez=20Vargas=2C=20Jos=C3=A9=20Pablo?= Date: Sun, 7 Sep 2025 01:16:07 -0600 Subject: [PATCH 3/3] tests: Add unit tests for unsubscription function for location.on() --- src/lib/core/LocationFull.test.ts | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/lib/core/LocationFull.test.ts b/src/lib/core/LocationFull.test.ts index c16ad41..70647d5 100644 --- a/src/lib/core/LocationFull.test.ts +++ b/src/lib/core/LocationFull.test.ts @@ -68,6 +68,36 @@ describe("LocationFull", () => { // Cleanup. unSub(); }); + test("Should unregister the provided callback when the returned function is called.", () => { + // Arrange. + const callback = vi.fn(); + const unSub = location.on('beforeNavigate', callback); + + // Act. + unSub(); + + // Assert. + globalThis.window.history.pushState(null, '', 'http://example.com/other'); + expect(callback).not.toHaveBeenCalled(); + }); + test("Should not affect other handlers when unregistering one of the event handlers.", () => { + // Arrange. + const callback1 = vi.fn(); + const callback2 = vi.fn(); + const unSub1 = location.on('beforeNavigate', callback1); + const unSub2 = location.on('beforeNavigate', callback2); + + // Act. + unSub1(); + + // Assert. + globalThis.window.history.pushState(null, '', 'http://example.com/other'); + expect(callback1).not.toHaveBeenCalled(); + expect(callback2).toHaveBeenCalledOnce(); + + // Cleanup. + unSub2(); + }); test.each([ { method: 'push',