Skip to content
Open
2 changes: 2 additions & 0 deletions index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,7 @@ declare module '@lightningjs/blits' {
export interface RouterHooks {
init?: () => Promise<> | void;
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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;
Copy link
Contributor

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).

Copy link
Contributor Author

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!

error?: (err: string) => string | Route | Promise<string | Route> | void;
}

Expand Down Expand Up @@ -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, fromComponent: ComponentBase) => string | Route | Promise<string | Route>;
}

export type Route = {
Expand Down
40 changes: 40 additions & 0 deletions src/router/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -500,6 +501,45 @@ export const navigate = async function () {
await setOrAnimate(holder, route.transition.in, shouldAnimate)
}
}

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if (this.parent[symbols.routerHooks]) {
const hooks = this.parent[symbols.routerHooks]
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed removeView() to await.

if (hooks.afterEach) {
try {
// Get the previous view before it's removed
const previousView =
Copy link
Contributor

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.

Copy link
Contributor Author

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.

!reuse && previousRoute && children.length > 1 ? children[children.length - 2] : null

await hooks.afterEach.call(
this.parent,
route, // to
view, // toComponent
previousRoute, // from
previousView // fromComponent
)
} catch (error) {
Log.error('Error in "AfterEach" Hook', error)
}
}
}

if (route.hooks.after) {
try {
// Get the previous view before it's removed
const previousView =
!reuse && previousRoute && children.length > 1 ? children[children.length - 2] : null

await route.hooks.after.call(
this.parent,
route, // to
view, // toComponent
previousRoute, // from
previousView // 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]
Expand Down