fix: Ensure LocationFull validates state data before relaying it to history API (#55)

* fix: Ensure LocationFull validates state data before relaying it to history API

* chore(tests):  Rename LocationFull's test file and do proper cleanup

* tests: Add unit tests for unsubscription function for location.on()

Author: webJose

src/lib/core/LocationFull.svelte.test.ts renamed to src/lib/core/LocationFull.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", () => {
@@ -58,14 +57,46 @@ 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');
-            flushSync();
 
             // Assert.
             expect(callback).toHaveBeenCalledOnce();
+
+            // 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([
             {
@@ -79,13 +110,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' };
-            location.on('beforeNavigate', callback);
+            const state = { path: { test: 'value' }, hash: {} };
+            const unSub = 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({
@@ -96,16 +126,18 @@ 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');
-            flushSync();
 
             // Assert.
             expect(callback).toHaveBeenCalledWith({
@@ -116,17 +148,20 @@ 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');
-            flushSync();
 
             // Assert.
             expect(callback).toHaveBeenCalledWith({
@@ -137,34 +172,66 @@ describe("LocationFull", () => {
                 cancelReason: 'test',
                 cancel: expect.any(Function)
             });
+
+            // Cleanup.
+            unSub1();
+            unSub2();
+            unSub3();
+        });
+        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();
-            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');
-            flushSync();
 
             // 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');
-            flushSync();
 
             // Assert.
             expect(callback).toHaveBeenCalledWith({ url: 'http://example.com/other', cause: 'test', method: 'push', state });
+
+            // Cleanup.
+            unSub1();
+            unSub2();
         });
     });
     describe('url', () => {
@@ -177,7 +244,6 @@ describe("LocationFull", () => {
 
             // Act.
             globalThis.window.history[fn](null, '', newUrl);
-            flushSync();
 
             // Assert.
             expect(location.url.href).toBe(newUrl);
@@ -193,12 +259,28 @@ describe("LocationFull", () => {
 
             // Act.
             globalThis.window.history[fn](state, '', 'http://example.com/new');
-            flushSync();
 
             // Assert.
             expect(location.getState(false)).toEqual(state.path);
             expect(location.getState(true)).toEqual(state.hash.single);
             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);
+        });
+    });
 });

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;