-
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?
Changes from all commits
85b01b5
a2a86ae
ba64e4c
6acc761
78d6698
737950a
d2078d3
3559bdf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -626,6 +626,7 @@ declare module '@lightningjs/blits' { | |
| export interface RouterHooks { | ||
| init?: () => Promise<> | void; | ||
| beforeEach?: (to: Route, from: Route) => string | Route | Promise<string | Route> | void; | ||
| afterEach?: (to: Route, toComponent: ComponentBase, from: Route | undefined, fromComponent: ComponentBase | null) => string | Route | Promise<string | Route> | void; | ||
| error?: (err: string) => string | Route | Promise<string | Route> | void; | ||
| } | ||
|
|
||
|
|
@@ -749,6 +750,7 @@ declare module '@lightningjs/blits' { | |
|
|
||
| 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>; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the signature of the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| } | ||
|
|
||
| export type Route = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -335,6 +335,7 @@ export const navigate = async function () { | |
| return | ||
| } | ||
| } | ||
|
|
||
| // add the previous route (technically still the current route at this point) | ||
| // into the history stack when inHistory is true and we're not navigating back | ||
| if ( | ||
|
|
@@ -476,15 +477,17 @@ export const navigate = async function () { | |
| } | ||
|
|
||
| let shouldAnimate = false | ||
| // Declare oldView in broader scope so it can be used in hooks below | ||
| let oldView = null | ||
|
|
||
| // apply out out transition on previous view if available, unless | ||
| // we're reusing the prvious page component | ||
| if (previousRoute !== undefined && reuse === false) { | ||
| // only animate when there is a previous route | ||
| shouldAnimate = true | ||
| const oldView = this[symbols.children].splice(1, 1).pop() | ||
| oldView = this[symbols.children].splice(1, 1).pop() | ||
| if (oldView) { | ||
| removeView(previousRoute, oldView, route.transition.out, navigatingBack) | ||
| await removeView(previousRoute, oldView, route.transition.out, navigatingBack) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -500,6 +503,37 @@ export const navigate = async function () { | |
| await setOrAnimate(holder, route.transition.in, shouldAnimate) | ||
| } | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| if (this.parent[symbols.routerHooks]) { | ||
| const hooks = this.parent[symbols.routerHooks] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed |
||
| if (hooks.afterEach) { | ||
| try { | ||
| await hooks.afterEach.call( | ||
| this.parent, | ||
| route, // to | ||
| view, // toComponent | ||
| previousRoute, // from | ||
| oldView // fromComponent | ||
| ) | ||
| } catch (error) { | ||
| Log.error('Error in "AfterEach" Hook', error) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (route.hooks.after) { | ||
| try { | ||
| await route.hooks.after.call( | ||
| this.parent, | ||
| route, // to | ||
| view, // toComponent | ||
| previousRoute, // from | ||
| oldView // fromComponent | ||
| ) | ||
| } catch (error) { | ||
| Log.error('Error or Rejected Promise in "After" Hook', error) | ||
| } | ||
| } | ||
| } else { | ||
| Log.error(`Route ${route.hash} not found`) | ||
| const routerHooks = 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.
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