Skip to content

Conversation

@ruggero-nardi
Copy link

Code change description:

  • Now when a "before" hook is executed, this is done inside a try-catch. This way the before hook will behave exactly as before when executed succesfully, and now it will catch errors and promise rejections.
  • In the catch execution a flag is set to true to prevent a navigate function execution, this way the browser history can be reverted.
  • the currentRoute is set back to the "previousRoute" to prevent incorrect future back() navigations.

Bug description:

When a route is set to do checks before navigating, the "before" hook works incorrectly when trying to prevent navigation.

  • True/false returned:
    • Documentation states that a boolean can be returned, but this is not possible and won't have any effect in the code.
  • String or Route returned:
    • OK
  • Promise<string|Route> returned:
    • If resolve(to): Navigates away to the new route.
    • If resolve(from): Navigates to the same current route (Refreshes the current page)
    • If reject(): Error, app stucked
    • If resolve(undefinedPath): Navigation prevented, Blits error shown in console.

@CLAassistant
Copy link

CLAassistant commented Oct 17, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@michielvandergeest michielvandergeest left a comment

Choose a reason for hiding this comment

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

thanks for this contribution @ruggero-nardi !

I've left a few inline comments, could you look at those? Also, this has been added for the route before hook, but not for the beforeEach hook. I guess the same behaviour when rejecting a promise would be expected there?

Other than that, great addition of functionality to the router. Thx!

Comment on lines 42 to 46
if (this[symbols.state].preventHashChangeNavigation) {
this[symbols.state].preventHashChangeNavigation = false
return
}
Router.navigate.apply(this)
Copy link
Collaborator

Choose a reason for hiding this comment

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

instead of adding an extra state variable (preventHashChangeNavigation) to the router view, wouldn't it be more clean to keep this contained in the router logic itself?

in that case we would just always call Router.navigate.apply(this) and check a prevent has change variable in the router in the navigate method

Comment on lines +272 to +276
if (isString(beforeHookOutput)) {
currentRoute = previousRoute
to(beforeHookOutput)
return
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some changes to this portion have been made by me around the same time as your PR (see: https://github.com/lightning-js/blits/pull/502/files). We should make sure that we keep that functionality in tact

Copy link
Author

Choose a reason for hiding this comment

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

Checked this, updated my dev branch with upstream dev branch and now it has all your changes plus the the additions to the router hooks

@github-actions
Copy link

Test Results: ✅ PASSED

Run at: 2025-10-21T08:03:31.469Z

Summary:
passed: 920 failed: 0 of 920 tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants