Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/website/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
"dependencies": {
"@fontsource-variable/geist": "^5.2.9",
"@fontsource-variable/geist-mono": "^5.2.8",
"@ilha/router": "^0.8.5",
"@ilha/router": "^0.8.6",
"@ilha/store": "^0.7.2",
"areia": "^0.1.36",
"dedent": "^1.7.2",
Expand Down
2 changes: 1 addition & 1 deletion bun.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion packages/router/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@ilha/router",
"version": "0.8.5",
"version": "0.8.6",
"description": "A tiny SPA router for Ilha",
"keywords": [
"frontend",
Expand Down
191 changes: 191 additions & 0 deletions packages/router/src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1411,7 +1411,7 @@
loader(async () => ({ nested: true, shared: "nested" })),
loader(async () => ({ page: true, shared: "page" })),
]);
const Page = ilha.render(({ input }: any) => `<p>ok</p>`);

Check warning on line 1414 in packages/router/src/index.test.ts

View workflow job for this annotation

GitHub Actions / Build, test & publish

eslint(no-unused-vars)

Parameter 'input' is declared but never used. Unused parameters should start with a '_'.
const r = router().route("/user", Page).attachLoader("/user", composed);
const result = await r.runLoader("/user");
expect(result).toEqual({
Expand Down Expand Up @@ -3041,3 +3041,194 @@
expect(result).not.toMatch(/<\/main>\s*<div data-slot="resizable-panel"/);
});
});

// ─────────────────────────────────────────────
// Same-pattern navigation — loaders re-run when only params/search change
// ─────────────────────────────────────────────

describe("same-pattern navigation re-runs loaders (SPA / client loader)", () => {
let el: Element;
let unmount: (() => void) | null = null;
let fetchSpy: ReturnType<typeof spyOn<typeof globalThis, "fetch">>;

const flush = async () => {
await new Promise((r) => setTimeout(r, 0));
await new Promise((r) => setTimeout(r, 0));
};

beforeEach(() => {
setLocation("/");
el = makeEl();
fetchSpy = (spyOn(globalThis, "fetch") as any).mockImplementation(async () => {
return new Response(JSON.stringify({ kind: "data", data: {} }), {
status: 200,
headers: { "content-type": "application/json" },
});
});
});

afterEach(() => {
unmount?.();
unmount = null;
fetchSpy.mockRestore();
cleanup(el);
setLocation("/");
});

const TablePage = ilha.render(
({ input }: any) => `<p>table:${input?.table ?? "?"}|page:${input?.page ?? "-"}</p>`,
);

function mountTableRouter(load: ReturnType<typeof mock>) {
unmount = router()
.route("/", HomePage)
.route("/about", AboutPage)
.route("/t/:name", TablePage, loader(load as any))
.mount(el);
}

it("re-runs the loader when only a param changes (/t/a → /t/b)", async () => {
const load = mock(async ({ params }: any) => ({ table: params.name }));
mountTableRouter(load);

navigate("/t/a");
await flush();
expect(el.innerHTML).toContain("table:a");

navigate("/t/b");
await flush();

expect(load).toHaveBeenCalledTimes(2);
expect(load.mock.calls[1]?.[0]?.params?.name).toBe("b");
expect(el.innerHTML).toContain("table:b");
});

it("re-runs the loader when only search changes (?page=1 → ?page=2)", async () => {
const load = mock(async ({ params, url }: any) => ({
table: params.name,
page: url.searchParams.get("page"),
}));
mountTableRouter(load);

navigate("/t/a?page=1");
await flush();
expect(el.innerHTML).toContain("page:1");

navigate("/t/a?page=2");
await flush();

expect(load).toHaveBeenCalledTimes(2);
expect(load.mock.calls[1]?.[0]?.url?.searchParams.get("page")).toBe("2");
expect(el.innerHTML).toContain("page:2");
});

it("does not re-run the loader for an identical URL", async () => {
const load = mock(async ({ params }: any) => ({ table: params.name }));
mountTableRouter(load);

navigate("/t/a");
await flush();
navigate("/t/a");
await flush();

expect(load).toHaveBeenCalledTimes(1);
});

it("does not re-run the loader for a hash-only change", async () => {
const load = mock(async ({ params }: any) => ({ table: params.name }));
mountTableRouter(load);

navigate("/t/a");
await flush();
navigate("/t/a#section");
await flush();

expect(load).toHaveBeenCalledTimes(1);
expect(el.innerHTML).toContain("table:a");
});

it("rapid same-pattern navigations mount only the final result", async () => {
const load = mock(async ({ params }: any) => ({ table: params.name }));
mountTableRouter(load);

navigate("/t/a");
navigate("/t/b");
navigate("/t/c");
await flush();
await flush();

expect(el.innerHTML).toContain("table:c");
expect(el.innerHTML).not.toContain("table:a");
expect(el.innerHTML).not.toContain("table:b");
});

it("cross-pattern navigation still remounts exactly once", async () => {
const load = mock(async ({ params }: any) => ({ table: params.name }));
mountTableRouter(load);

navigate("/t/a");
await flush();
navigate("/about");
await flush();

expect(el.innerHTML).toContain("about");
expect(load).toHaveBeenCalledTimes(1);
});
});

describe("same-pattern navigation re-runs loaders (hydrate / server loader)", () => {
const flush = async () => {
await new Promise((r) => setTimeout(r, 0));
await new Promise((r) => setTimeout(r, 0));
};

it("re-fetches loader data from the endpoint on param-only and search-only navigations", async () => {
const TablePage = ilha.render(
({ input }: any) => `<p>table:${input?.table ?? "?"}|page:${input?.page ?? "-"}</p>`,
);
const reg = { home: HomePage, table: TablePage };

const requested: string[] = [];
const fetchSpy = (spyOn(globalThis, "fetch") as any).mockImplementation(async (url: any) => {
const u = new URL(String(url), "http://localhost");
const path = u.searchParams.get("path") ?? "";
requested.push(path);
const target = new URL(path, "http://localhost");
const table = target.pathname.split("/").pop();
return new Response(
JSON.stringify({
kind: "data",
data: { table, page: target.searchParams.get("page") },
}),
{ status: 200, headers: { "content-type": "application/json" } },
);
});

setLocation("/t/a");
const el = makeEl(`<div data-router-view><p>table:a|page:-</p></div>`);

// markLoader simulates the SSR-split client graph: hasLoader without the
// loader function itself, so navigation must fetch the loader endpoint.
const unmount = router()
.route("/", HomePage)
.route("/t/:name", TablePage)
.markLoader("/t/:name")
.mount(el, { hydrate: true, registry: reg });
await flush();

navigate("/t/b");
await flush();
expect(requested).toContain("/t/b");
expect(el.innerHTML).toContain("table:b");

navigate("/t/b?page=2");
await flush();
expect(requested).toContain("/t/b?page=2");
expect(el.innerHTML).toContain("page:2");

unmount();
cleanup(el);
fetchSpy.mockRestore();
setLocation("/");
});
});
33 changes: 28 additions & 5 deletions packages/router/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ export type Loader<T> = (ctx: LoaderContext) => Promise<T> | T;
/**
* Identity function for declaring a loader. Exists purely as a type anchor and
* a marker for the Vite plugin to detect by export name.
*
* Loaders must read `ctx.params`/`ctx.url` rather than `useRoute()` — the
* route store still holds the previous route while a navigation's loader is
* in flight, so `useRoute().params()` inside a loader reads stale params.
*/
export function loader<T>(fn: Loader<T>): Loader<T> {
return fn;
Expand Down Expand Up @@ -2349,14 +2353,22 @@ export function router(options: RouterOptions = {}): RouterBuilder {
// destroying the event listeners and signal bindings ilha.mount() wired up.
const viewHost = host.querySelector<Element>("[data-router-view]") ?? host;
let currentMountedIsland: Island<any, any> | null = activeIsland();
// Logical URL (pathname + search, no hash) the view was mounted for —
// same-pattern navigations keep the island identity but must still
// re-run loaders when params or search change.
let currentMountedPath: string = routePath() + routeSearch();

const reverseRegistry = registry ? buildReverseRegistry(registry) : undefined;

let navVersion = 0;

const NavHandler = ilha.render((): string => {
const current = activeIsland();
if (current !== currentMountedIsland) {
// Read the route-store signals (not location directly) so this
// handler re-runs on param/search-only navigations. Hash excluded —
// fragment changes must not re-run loaders.
const pathWithSearch = routePath() + routeSearch();
if (current !== currentMountedIsland || pathWithSearch !== currentMountedPath) {
const thisNav = ++navVersion;
// Cancel any in-flight loader fetch for the previous nav
navAbort?.abort();
Expand All @@ -2369,11 +2381,10 @@ export function router(options: RouterOptions = {}): RouterBuilder {
unmountView?.();
unmountView = null;
try {
const loc = getAdapter().readLocation();
unmountView = await mountRouteWithHydration(
current,
viewHost,
loc.pathname + loc.search,
pathWithSearch,
signal,
registry,
reverseRegistry,
Expand All @@ -2389,6 +2400,7 @@ export function router(options: RouterOptions = {}): RouterBuilder {
settle();
}
currentMountedIsland = current;
currentMountedPath = pathWithSearch;
});
}
return "";
Expand Down Expand Up @@ -2454,10 +2466,11 @@ export function router(options: RouterOptions = {}): RouterBuilder {
const settle = beginNavigation();
try {
const loc = getAdapter().readLocation();
const pathWithSearch = loc.pathname + loc.search;
const um = await mountRouteWithHydration(
island,
viewHost,
loc.pathname + loc.search,
pathWithSearch,
ac.signal,
registry,
reverseRegistry,
Expand All @@ -2466,6 +2479,7 @@ export function router(options: RouterOptions = {}): RouterBuilder {
unmountView?.();
unmountView = um;
currentMountedIsland = island;
currentMountedPath = pathWithSearch;
} else {
um();
}
Expand Down Expand Up @@ -2495,6 +2509,10 @@ export function router(options: RouterOptions = {}): RouterBuilder {
// SPA mode — RouterView renders HTML but islands need .mount() for interactivity.
let unmountIsland: (() => void) | null = null;
let currentMountedIsland: Island<any, any> | null = null;
// Logical URL (pathname + search, no hash) the view was mounted for —
// same-pattern navigations keep the island identity but must still
// re-run loaders when params or search change.
let currentMountedPath: string | null = null;
let navVersion = 0;

unmountView = RouterView.mount(host);
Expand All @@ -2511,6 +2529,7 @@ export function router(options: RouterOptions = {}): RouterBuilder {
unmountIsland?.();
unmountIsland = null;
currentMountedIsland = island;
currentMountedPath = routePath() + routeSearch();
if (!island) {
// RouterView rendered the 404 island's HTML statically; mount it so
// it gets a real island lifecycle (events, effects) like any route.
Expand Down Expand Up @@ -2579,7 +2598,11 @@ export function router(options: RouterOptions = {}): RouterBuilder {

const NavHandler = ilha.render((): string => {
const current = activeIsland();
if (current !== currentMountedIsland) {
// Read the route-store signals (not location directly) so this
// handler re-runs on param/search-only navigations. Hash excluded —
// fragment changes must not re-run loaders.
const pathWithSearch = routePath() + routeSearch();
if (current !== currentMountedIsland || pathWithSearch !== currentMountedPath) {
const thisNav = ++navVersion;
navAbort?.abort();
navAbort = new AbortController();
Expand Down
9 changes: 9 additions & 0 deletions packages/store/src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1027,7 +1027,7 @@
.derived("user", async (ctx) => {
runs++;
lastSignal = ctx.signal;
ctx.get().id;

Check warning on line 1030 in packages/store/src/index.test.ts

View workflow job for this annotation

GitHub Actions / Build, test & publish

eslint(no-unused-expressions)

Expected expression to be used
return new Promise<never>(() => {}); // never settles
})
.build();
Expand Down Expand Up @@ -1335,6 +1335,15 @@
stop();
});

it("hydration is synchronous — getState() immediately after persist() sees stored data", () => {
window.localStorage.setItem("p1b", JSON.stringify({ count: 42, label: "restored" }));
const s = store({ count: 0, label: "" }).build();
const stop = persist(s, "p1b");
// Eager, non-reactive read right after persist() — no effect/subscribe involved.
expect(s.getState()).toEqual({ count: 42, label: "restored" });
stop();
});
Comment on lines +1341 to +1345

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Guarantee persistence cleanup when the assertion fails.

If the assertion throws, stop() is skipped, leaving the subscription and storage event listener registered for subsequent tests. Wrap the assertion in try/finally.

Proposed fix
     const stop = persist(s, "p1b");
-    // Eager, non-reactive read right after persist() — no effect/subscribe involved.
-    expect(s.getState()).toEqual({ count: 42, label: "restored" });
-    stop();
+    try {
+      // Eager, non-reactive read right after persist() — no effect/subscribe involved.
+      expect(s.getState()).toEqual({ count: 42, label: "restored" });
+    } finally {
+      stop();
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const stop = persist(s, "p1b");
// Eager, non-reactive read right after persist() — no effect/subscribe involved.
expect(s.getState()).toEqual({ count: 42, label: "restored" });
stop();
});
const stop = persist(s, "p1b");
try {
// Eager, non-reactive read right after persist() — no effect/subscribe involved.
expect(s.getState()).toEqual({ count: 42, label: "restored" });
} finally {
stop();
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/store/src/index.test.ts` around lines 1341 - 1345, Wrap the
assertion following persist(s, "p1b") in a try/finally block so stop() always
executes, including when expect(s.getState()) fails. Keep the existing state
expectation unchanged and place the cleanup call in the finally block.


it("writes state on every commit", () => {
const s = store({ count: 0 }).build();
const stop = persist(s, "p2");
Expand Down
2 changes: 1 addition & 1 deletion templates/elysia/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
},
"dependencies": {
"@elysiajs/static": "^1.4.10",
"@ilha/router": "^0.8.5",
"@ilha/router": "^0.8.6",
"@ilha/store": "^0.7.2",
"areia": "^0.1.36",
"elysia": "^1.4.29",
Expand Down
2 changes: 1 addition & 1 deletion templates/hono/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
},
"dependencies": {
"@hono/node-server": "^2.0.8",
"@ilha/router": "^0.8.5",
"@ilha/router": "^0.8.6",
"@ilha/store": "^0.7.2",
"areia": "^0.1.36",
"hono": "^4.12.28",
Expand Down
2 changes: 1 addition & 1 deletion templates/nitro/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"preview": "vite preview"
},
"dependencies": {
"@ilha/router": "^0.8.5",
"@ilha/router": "^0.8.6",
"@ilha/store": "^0.7.2",
"areia": "^0.1.36",
"ilha": "^0.9.2",
Expand Down
2 changes: 1 addition & 1 deletion templates/vite/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"preview": "vite preview"
},
"dependencies": {
"@ilha/router": "^0.8.5",
"@ilha/router": "^0.8.6",
"@ilha/store": "^0.7.2",
"areia": "^0.1.36",
"ilha": "^0.9.2",
Expand Down
Loading