From bfb9623d3a2b4a02cd91eb6a0b4942f9a83898d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ramirez=20Vargas=2C=20Jos=C3=A9=20Pablo?= Date: Sun, 7 Sep 2025 14:09:44 -0600 Subject: [PATCH] fix: Ensure the cleanup function from init() really rolls back everything --- src/lib/core/Logger.test.ts | 51 +++++++- src/lib/core/Logger.ts | 11 +- src/lib/core/options.test.ts | 22 +++- src/lib/core/options.ts | 16 ++- src/lib/core/trace.svelte.ts | 16 ++- src/lib/core/trace.test.ts | 28 ++++- src/lib/index.test.ts | 217 ++++++++++++++++++++++++++++++++++- src/lib/index.ts | 9 +- 8 files changed, 355 insertions(+), 15 deletions(-) diff --git a/src/lib/core/Logger.test.ts b/src/lib/core/Logger.test.ts index 22f04d2..caa13a8 100644 --- a/src/lib/core/Logger.test.ts +++ b/src/lib/core/Logger.test.ts @@ -1,10 +1,14 @@ import { describe, test, expect, beforeEach, afterEach, vi } from "vitest"; -import { logger, setLogger } from "./Logger.js"; +import { logger, resetLogger, setLogger } from "./Logger.js"; import type { ILogger } from "$lib/types.js"; describe("logger", () => { - test("Should default to globalThis.console", () => { - expect(logger).toBe(globalThis.console); + test("Should default to offLogger (not console)", () => { + expect(logger).not.toBe(globalThis.console); + expect(logger.debug).toBeDefined(); + expect(logger.log).toBeDefined(); + expect(logger.warn).toBeDefined(); + expect(logger.error).toBeDefined(); }); }); @@ -158,4 +162,45 @@ describe("setLogger", () => { expect(mockLogger.error).toHaveBeenCalledWith("error", { error: "object" }); }); }); + + describe("resetLogger", () => { + test("Should reset logger to offLogger (default uninitialized state).", () => { + // Arrange - Set logger to console + setLogger(true); + expect(logger).toBe(globalThis.console); + + // Act + resetLogger(); + + // Assert - Logger should be back to offLogger + expect(logger).not.toBe(globalThis.console); + expect(logger.debug).toBeDefined(); + expect(logger.log).toBeDefined(); + expect(logger.warn).toBeDefined(); + expect(logger.error).toBeDefined(); + }); + + test("Should reset logger from custom logger to offLogger.", () => { + // Arrange - Set logger to custom logger + const customLogger = { + debug: vi.fn(), + log: vi.fn(), + warn: vi.fn(), + error: vi.fn() + }; + setLogger(customLogger); + expect(logger).toBe(customLogger); + + // Act + resetLogger(); + + // Assert - Logger should be back to offLogger + expect(logger).not.toBe(customLogger); + expect(logger).not.toBe(globalThis.console); + expect(logger.debug).toBeDefined(); + expect(logger.log).toBeDefined(); + expect(logger.warn).toBeDefined(); + expect(logger.error).toBeDefined(); + }); + }); }); diff --git a/src/lib/core/Logger.ts b/src/lib/core/Logger.ts index cad74b5..bb07ec9 100644 --- a/src/lib/core/Logger.ts +++ b/src/lib/core/Logger.ts @@ -11,8 +11,15 @@ const offLogger: ILogger = { error: noop }; -export let logger: ILogger = stockLogger; +export let logger: ILogger = offLogger; export function setLogger(newLogger: boolean | ILogger) { logger = newLogger === true ? stockLogger : (newLogger === false ? offLogger : newLogger); -}; +} + +/** + * Resets the logger to the default uninitialized state (offLogger). + */ +export function resetLogger(): void { + logger = offLogger; +} diff --git a/src/lib/core/options.test.ts b/src/lib/core/options.test.ts index 2d45eae..04551f0 100644 --- a/src/lib/core/options.test.ts +++ b/src/lib/core/options.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "vitest"; -import { routingOptions } from "./options.js"; +import { resetRoutingOptions, routingOptions } from "./options.js"; describe("options", () => { test("The routing options' initial values are the expected ones.", () => { @@ -94,4 +94,24 @@ describe("options", () => { // Restore original value routingOptions.implicitMode = originalValue; }); + + test("Should reset all options to defaults when resetRoutingOptions is called.", () => { + // Arrange - Modify all options to non-default values + routingOptions.full = true; + routingOptions.hashMode = 'multi'; + routingOptions.implicitMode = 'hash'; + + // Verify they were changed + expect(routingOptions.full).toBe(true); + expect(routingOptions.hashMode).toBe('multi'); + expect(routingOptions.implicitMode).toBe('hash'); + + // Act + resetRoutingOptions(); + + // Assert - Verify all options are back to defaults + expect(routingOptions.full).toBe(false); + expect(routingOptions.hashMode).toBe('single'); + expect(routingOptions.implicitMode).toBe('path'); + }); }); diff --git a/src/lib/core/options.ts b/src/lib/core/options.ts index 9643ca6..d1680d5 100644 --- a/src/lib/core/options.ts +++ b/src/lib/core/options.ts @@ -47,10 +47,22 @@ export type RoutingOptions = { } /** - * Global routing options. + * Default routing options used for rollback. */ -export const routingOptions: Required = { +const defaultRoutingOptions: Required = { full: false, hashMode: 'single', implicitMode: 'path' }; + +/** + * Global routing options. + */ +export const routingOptions: Required = structuredClone(defaultRoutingOptions); + +/** + * Resets routing options to their default values. + */ +export function resetRoutingOptions(): void { + Object.assign(routingOptions, structuredClone(defaultRoutingOptions)); +} diff --git a/src/lib/core/trace.svelte.ts b/src/lib/core/trace.svelte.ts index dd35c8b..dd8b791 100644 --- a/src/lib/core/trace.svelte.ts +++ b/src/lib/core/trace.svelte.ts @@ -74,12 +74,24 @@ export function getAllChildRouters(parent: RouterEngine) { } /** - * Tracing options that can be set during library initialization. + * Default tracing options used for rollback. */ -export const traceOptions: TraceOptions = { +const defaultTraceOptions: TraceOptions = { routerHierarchy: false, }; +/** + * Tracing options that can be set during library initialization. + */ +export const traceOptions: TraceOptions = structuredClone(defaultTraceOptions); + export function setTraceOptions(options?: typeof traceOptions) { Object.assign(traceOptions, options); } + +/** + * Resets tracing options to their default values. + */ +export function resetTraceOptions(): void { + Object.assign(traceOptions, structuredClone(defaultTraceOptions)); +} diff --git a/src/lib/core/trace.test.ts b/src/lib/core/trace.test.ts index 65d5edc..100b67b 100644 --- a/src/lib/core/trace.test.ts +++ b/src/lib/core/trace.test.ts @@ -1,5 +1,5 @@ import { afterAll, beforeAll, beforeEach, describe, expect, test, vi } from "vitest"; -import { getAllChildRouters, setTraceOptions, traceOptions } from "./trace.svelte.js"; +import { getAllChildRouters, resetTraceOptions, setTraceOptions, traceOptions } from "./trace.svelte.js"; import { RouterEngine } from "./RouterEngine.svelte.js"; import { init } from "$lib/index.js"; @@ -29,6 +29,32 @@ describe('setTraceOptions', () => { }); }); +describe('resetTraceOptions', () => { + test("Should reset trace options to defaults.", () => { + // Arrange - Set to non-default value + setTraceOptions({ routerHierarchy: true }); + expect(traceOptions.routerHierarchy).toBe(true); + + // Act + resetTraceOptions(); + + // Assert - Should be back to default + expect(traceOptions.routerHierarchy).toBe(false); + }); + + test("Should reset trace options when already at defaults.", () => { + // Arrange - Ensure it's already at default + resetTraceOptions(); + expect(traceOptions.routerHierarchy).toBe(false); + + // Act + resetTraceOptions(); + + // Assert - Should still be at default + expect(traceOptions.routerHierarchy).toBe(false); + }); +}); + describe('registerRouter', () => { let cleanup: () => void; beforeAll(() => { diff --git a/src/lib/index.test.ts b/src/lib/index.test.ts index c3fbcd4..1f0567e 100644 --- a/src/lib/index.test.ts +++ b/src/lib/index.test.ts @@ -1,6 +1,16 @@ -import { describe, expect, test } from "vitest"; +import { describe, expect, test, beforeEach, afterEach } from "vitest"; +import { logger } from "./core/Logger.js"; +import { routingOptions } from "./core/options.js"; +import { traceOptions } from "./core/trace.svelte.js"; +import { location } from "./core/Location.js"; describe('index', () => { + let cleanup: (() => void) | null = null; + + afterEach(() => { + cleanup?.(); + }); + test("Should export exactly the expected objects.", async () => { // Arrange. const expectedList = [ @@ -27,4 +37,209 @@ describe('index', () => { expect(expectedList.includes(key), `The library exports object ${key}, which is not expected.`).toEqual(true); } }); + + test("Should use offLogger as default uninitialized logger.", () => { + // Assert. + expect(logger.debug).toBeDefined(); + expect(logger.log).toBeDefined(); + expect(logger.warn).toBeDefined(); + expect(logger.error).toBeDefined(); + + // The default logger should be offLogger (no-op functions) + // We can't test the exact identity, but we can test that it's not the console + expect(logger).not.toBe(globalThis.console); + }); + + test("Should have default routing options in uninitialized state.", () => { + // Assert. + expect(routingOptions.full).toBe(false); + expect(routingOptions.hashMode).toBe('single'); + expect(routingOptions.implicitMode).toBe('path'); + }); + + test("Should have default trace options in uninitialized state.", () => { + // Assert. + expect(traceOptions.routerHierarchy).toBe(false); + }); + + test("Should have no location in uninitialized state.", () => { + // Assert. + expect(location).toBeUndefined(); + }); + + test("Should initialize with console logger when logger option is true (default).", async () => { + // Arrange. + const { init } = await import('./index.js'); + + // Act. + cleanup = init(); + + // Assert. + expect(logger).toBe(globalThis.console); + }); + + test("Should initialize with custom options and rollback properly.", async () => { + // Arrange. + const { init } = await import('./index.js'); + const customLogger = { + debug: () => {}, + log: () => {}, + warn: () => {}, + error: () => {} + }; + + // Capture initial state + const initialLoggerIsOffLogger = logger !== globalThis.console; + const initialRoutingOptions = { + full: routingOptions.full, + hashMode: routingOptions.hashMode, + implicitMode: routingOptions.implicitMode + }; + const initialTraceOptions = { + routerHierarchy: traceOptions.routerHierarchy + }; + + // Act - Initialize with custom options + cleanup = init({ + full: true, + hashMode: 'multi', + implicitMode: 'hash', + logger: customLogger, + trace: { + routerHierarchy: true + } + }); + + // Assert - Check that options were applied + expect(logger).toBe(customLogger); + expect(routingOptions.full).toBe(true); + expect(routingOptions.hashMode).toBe('multi'); + expect(routingOptions.implicitMode).toBe('hash'); + expect(traceOptions.routerHierarchy).toBe(true); + expect(location).toBeDefined(); + + // Act - Cleanup + cleanup(); + cleanup = null; + + // Assert - Check that everything was rolled back + expect(logger !== globalThis.console).toBe(initialLoggerIsOffLogger); // Back to offLogger + expect(routingOptions.full).toBe(initialRoutingOptions.full); + expect(routingOptions.hashMode).toBe(initialRoutingOptions.hashMode); + expect(routingOptions.implicitMode).toBe(initialRoutingOptions.implicitMode); + expect(traceOptions.routerHierarchy).toBe(initialTraceOptions.routerHierarchy); + expect(location).toBeNull(); + }); + + test("Should rollback routing options to defaults.", async () => { + // Arrange. + const { init } = await import('./index.js'); + + // Act - Initialize with non-default options + cleanup = init({ + full: true, + hashMode: 'multi', + implicitMode: 'hash' + }); + + // Verify options were applied + expect(routingOptions.full).toBe(true); + expect(routingOptions.hashMode).toBe('multi'); + expect(routingOptions.implicitMode).toBe('hash'); + + // Act - Cleanup + cleanup(); + cleanup = null; + + // Assert - Check that routing options were reset to defaults + expect(routingOptions.full).toBe(false); + expect(routingOptions.hashMode).toBe('single'); + expect(routingOptions.implicitMode).toBe('path'); + }); + + test("Should rollback logger to offLogger.", async () => { + // Arrange. + const { init } = await import('./index.js'); + + // Act - Initialize with console logger (default) + cleanup = init(); + + // Verify logger was set to console + expect(logger).toBe(globalThis.console); + + // Act - Cleanup + cleanup(); + cleanup = null; + + // Assert - Check that logger was reset to offLogger + expect(logger).not.toBe(globalThis.console); + expect(logger.debug).toBeDefined(); + expect(logger.log).toBeDefined(); + expect(logger.warn).toBeDefined(); + expect(logger.error).toBeDefined(); + }); + + test("Should rollback trace options to defaults.", async () => { + // Arrange. + const { init } = await import('./index.js'); + + // Act - Initialize with non-default trace options + cleanup = init({ + trace: { + routerHierarchy: true + } + }); + + // Verify trace options were applied + expect(traceOptions.routerHierarchy).toBe(true); + + // Act - Cleanup + cleanup(); + cleanup = null; + + // Assert - Check that trace options were reset to defaults + expect(traceOptions.routerHierarchy).toBe(false); + }); + + test("Should handle multiple init/cleanup cycles properly.", async () => { + // Arrange. + const { init } = await import('./index.js'); + + // Act & Assert - First cycle + cleanup = init({ full: true }); + expect(routingOptions.full).toBe(true); + expect(logger).toBe(globalThis.console); + expect(location).toBeDefined(); + + cleanup(); + cleanup = null; + expect(routingOptions.full).toBe(false); + expect(logger).not.toBe(globalThis.console); + expect(location).toBeNull(); + + // Act & Assert - Second cycle + cleanup = init({ hashMode: 'multi' }); + expect(routingOptions.hashMode).toBe('multi'); + expect(logger).toBe(globalThis.console); + expect(location).toBeDefined(); + + cleanup(); + cleanup = null; + expect(routingOptions.hashMode).toBe('single'); + expect(logger).not.toBe(globalThis.console); + expect(location).toBeNull(); + }); + + test("Should handle cleanup without previous initialization gracefully.", async () => { + // Arrange. + const { init } = await import('./index.js'); + cleanup = init(); + + // Act - Call cleanup function + const cleanupFn = cleanup; + cleanup = null; + + // Should not throw when calling cleanup + expect(() => cleanupFn()).not.toThrow(); + }); }); diff --git a/src/lib/index.ts b/src/lib/index.ts index 29d840e..417e119 100644 --- a/src/lib/index.ts +++ b/src/lib/index.ts @@ -1,9 +1,9 @@ import { setLocation } from "./core/Location.js"; import { LocationFull } from "./core/LocationFull.js"; import { LocationLite } from "./core/LocationLite.svelte.js"; -import { setLogger } from "./core/Logger.js"; -import { routingOptions, type RoutingOptions } from "./core/options.js"; -import { setTraceOptions, type TraceOptions } from "./core/trace.svelte.js"; +import { resetLogger, setLogger } from "./core/Logger.js"; +import { resetRoutingOptions, routingOptions, type RoutingOptions } from "./core/options.js"; +import { resetTraceOptions, setTraceOptions, type TraceOptions } from "./core/trace.svelte.js"; import type { ILogger } from "./types.js"; /** @@ -49,6 +49,9 @@ export function init(options?: InitOptions): () => void { return () => { newLocation?.dispose(); setLocation(null); + resetRoutingOptions(); + resetLogger(); + resetTraceOptions(); }; }