-
Notifications
You must be signed in to change notification settings - Fork 34
feat(router): add afterEach hook to RouterHooks for post-navigation handling #550
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
Test Results: ✅ PASSEDRun at: 2025-12-02T12:32:50.640Z Summary: |
Test Results: ✅ PASSEDRun at: 2025-12-04T11:52:20.264Z Summary: |
Test Results: ✅ PASSEDRun at: 2025-12-04T14:02:35.159Z Summary: |
Test Results: ✅ PASSEDRun at: 2025-12-18T14:14:47.719Z Summary: |
Test Results: ✅ PASSEDRun at: 2025-12-18T14:23:07.765Z Summary: |
| } | ||
|
|
||
| if (this.parent[symbols.routerHooks]) { | ||
| const hooks = this.parent[symbols.routerHooks] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removeView() isn’t awaited, so “after” hooks can run while the exit transition / cleanup is still in progress. Consider awaiting cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed removeView() to await.
src/router/router.js
Outdated
| if (hooks.afterEach) { | ||
| try { | ||
| // Get the previous view before it's removed | ||
| const previousView = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previousView is derived from children after mutation, which can be fragile and may resolve to the root container. Capturing the removed oldView would be more reliable. Same applies to the route.hooks.after block below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used oldView instead of previousView. Thanks.
index.d.ts
Outdated
| export interface RouterHooks { | ||
| init?: () => Promise<> | void; | ||
| beforeEach?: (to: Route, from: Route) => string | Route | Promise<string | Route> | void; | ||
| afterEach?: (to: Route, toComponent: ComponentBase, from: Route, fromComponent: ComponentBase) => string | Route | Promise<string | Route> | void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing: from / fromComponent can be undefined / null at runtime (first nav, reuse). Consider changing both afterEach and after to from?: Route and fromComponent: ComponentBase | null (or guarantee non-null in implementation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type definition changed as requested. Thanks!
Test Results: ✅ PASSEDRun at: 2025-12-19T17:42:51.845Z Summary: |
… null for from parameters
Test Results: ✅ PASSEDRun at: 2025-12-19T17:48:01.742Z Summary: |
Test Results: ✅ PASSEDRun at: 2025-12-19T17:52:25.069Z Summary: |
| } | ||
|
|
||
| export interface RouterHooks { | ||
| init?: () => Promise<> | void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Promise<> is invalid TypeScript syntax; use Promise instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| await setOrAnimate(holder, route.transition.in, shouldAnimate) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 498: route.transition.length is undefined because route.transition is an object.
Using transition.in.length - 1 ensures the last in-transition is awaited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
|
||
| export interface RouteHooks { | ||
| before?: (to: Route, from: Route) => string | Route | Promise<string | Route>; | ||
| after?: (to: Route, toComponent: ComponentBase, from: Route | undefined, fromComponent: ComponentBase | null) => string | Route | Promise<string | Route>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the signature of the after hook not the same as the before hook, with just to and from? That would make more sense to me and make the API more predictable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My goal is to use the after navigation hook to trigger shared, post-navigation API calls from the destination component (toComponent).
For example, if a component implements an analytics API like sendAnalytics(), we can automatically call it after the route change completes—so each page can own its own analytics logic, while the hook handles the common wiring.
Example:
after(to, toComponent, from, fromComponent) {
// Send analytics after navigating to the new page,
// but only if the destination component exposes `sendAnalytics`.
toComponent?.sendAnalytics?.();
}If you want I can remove new params.
Summary
Adds a new afterEach hook to the router that executes after navigation completes, providing access to both the route objects and their corresponding component instances.
Changes