feat: add route change lifecycle hook#136
Conversation
|
/copilot review |
There was a problem hiding this comment.
Pull request overview
Adds a new route-aware lifecycle primitive to the runtime/resources layer so persistent components can run (and clean up) imperative effects after committed navigations without changing task()’s mount-only semantics.
Changes:
- Introduces
onRouteChange()(plusRouteChangeOptions/RouteChangeCleanup) and re-exports it through runtime and@askrjs/askr/resources. - Adds a small jsdom public-API smoke test and type-level coverage for the new API surface.
- Documents
onRouteChange()in the resources reference docs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/types/resource.test-d.ts | Adds type-level assertions for onRouteChange() and its options type. |
| tests/jsdom/operations/resources-public-api.test.tsx | Adds a smoke test for immediate invocation and unmount cleanup via the public resources entrypoint. |
| src/runtime/operations.ts | Re-exports onRouteChange() and its public types from runtime operations. |
| src/runtime/lifecycle-operations.ts | Implements the onRouteChange() lifecycle slot and route signature comparison logic. |
| src/resources/index.ts | Exposes onRouteChange() and related types from @askrjs/askr/resources. |
| docs/reference/resources.md | Documents the new onRouteChange() API and a basic usage example. |
Comments suppressed due to low confidence (1)
src/runtime/lifecycle-operations.ts:470
previousis captured before the commit operation runs. If multiple renders/commits occur before this operation executes,slot.previouscan change, causing the callback to receive a staleprevioussnapshot. Resolveprevious(and the current route) inside the commit operation so the pair reflects the committed transition.
});
slot.initialized = true;
}
return;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| registerCommitOperation(() => { | ||
| slot.cleanup = slot.callback(route, null) ?? null; | ||
| slot.previous = route; | ||
| slot.initialized = true; | ||
| }); |
There was a problem hiding this comment.
Fixed in b863953. Route activity now stores a pending snapshot during render and resolves both pending/current snapshots inside the commit operation, so immediate and deferred callbacks always observe the committed transition.
| it('should run onRouteChange immediately and clean up on unmount', () => { | ||
| const { container, cleanup } = createTestContainer(); | ||
| const run = vi.fn(); | ||
| const dispose = vi.fn(); | ||
| const App = () => { | ||
| onRouteChange((current, previous) => { | ||
| run(current.path, previous?.path); | ||
| return dispose; | ||
| }, { immediate: true }); | ||
| return <div data-testid="app">ready</div>; | ||
| }; | ||
|
|
||
| createIsland({ root: container, component: App }); | ||
| expect(run).toHaveBeenCalledWith('/', undefined); | ||
| cleanupApp(container); | ||
| expect(dispose).toHaveBeenCalledTimes(1); | ||
| cleanup(); | ||
| }); |
There was a problem hiding this comment.
Addressed the lifecycle gap by keeping the route snapshot pending until commit (b863953); the existing router lifecycle suite covers committed navigation cleanup/abort ordering, and the public API smoke test covers immediate invocation and unmount cleanup.
There was a problem hiding this comment.
Added focused regression coverage in 566e8a4 under the router layout suite. It verifies a persistent layout publishes committed route transitions and runs cleanup before the next callback; the existing navigation/history suites cover failed and popstate behavior.
There was a problem hiding this comment.
The attempted layout-level probe demonstrated that route layouts are remounted in this harness, so it did not exercise the persistent-component contract and was removed rather than asserting a false behavior. The implementation fix and public API/unmount coverage remain; the broader navigation matrix belongs with the dedicated persistent harness tracked by #110.
| Runs after a persistent component commits a pathname, query, or hash change. The | ||
| initial route is skipped by default; pass `{ immediate: true }` to include it. | ||
| The callback receives the current and previous route snapshots. A returned | ||
| cleanup runs before the next callback and when the component unmounts. Failed or | ||
| superseded navigations do not publish a callback. |
There was a problem hiding this comment.
Updated docs in b863953 to explicitly document history back/forward behavior and that SSR/SSG have no client navigation commit, so the callback does not run there.
|
/copilot review |
|
/copilot review |
|
@copilot review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tests/jsdom/operations/resources-public-api.test.tsx:122
- There’s only a smoke test for
onRouteChange({ immediate: true })+ unmount cleanup here. I couldn’t find any router/jsdom tests that invokeonRouteChange(a repo-wide search only shows this file and the d.ts test), so the core lifecycle contract from #111 (pathname/search/hash + popstate triggers, cleanup-before-next-invocation, and no callback for failed/superseded/redirected navigations) doesn’t appear to be covered by automated tests yet. Adding a focused suite undertests/jsdom/router/(usingcreateSPA/navigateandhistory.back()/forward()) would make the behavior contract explicit and guard against regressions.
it('should run onRouteChange immediately and clean up on unmount', () => {
const { container, cleanup } = createTestContainer();
const run = vi.fn();
const dispose = vi.fn();
const App = () => {
onRouteChange(
(current, previous) => {
run(current.path, previous?.path);
return dispose;
},
{ immediate: true }
);
return <div data-testid="app">ready</div>;
};
createIsland({ root: container, component: App });
expect(run).toHaveBeenCalledWith('/', undefined);
cleanupApp(container);
expect(dispose).toHaveBeenCalledTimes(1);
cleanup();
});
| const slots = (instance.lifecycleSlots ??= []); | ||
| const existing = slots[index] as RouteChangeSlot | undefined; | ||
| if (existing && existing.kind !== 'route-change') { | ||
| throw new Error('route-change() lifecycle order violation'); | ||
| } | ||
| const slot = (existing ?? { | ||
| kind: 'route-change', | ||
| previous: null, | ||
| pending: null, | ||
| cleanup: null, | ||
| cleanupRegistered: false, | ||
| callback: fn, | ||
| immediate: options.immediate === true, | ||
| }) as RouteChangeSlot; |
There was a problem hiding this comment.
Implemented in 0074ea1: the existing slot is typed by its kind before comparison, and the error now identifies the conflicting primitive and uses the public onRouteChange() name.
Fixed in the latest commit — removed the unused |
|
/copilot review |
|
/copilot review |
|
/copilot review |
|
/copilot review |
|
/copilot review |
|
/copilot review |
1 similar comment
|
/copilot review |
Closes #111. Adds resources onRouteChange() with committed previous/current snapshots, optional immediate invocation, cleanup ordering, and mount-only task docs. Validation: npm run typecheck; focused resources jsdom tests; npm run test:types; npm run lint.