Skip to content

Conversation

@suresh-gangumalla
Copy link
Collaborator

  • Updated Router configuration to include router hooks along with routes from Application instance
  • Executing router beforeAll hook on every route navigation

- Updated Router configuration to include router hooks
along with routes from Application instance
- Executing router beforeAll hook on every route navigation

Signed-off-by: Suresh Kumar Gangumalla <[email protected]>
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.

This code looks good, but as far as I understand it's now only possible to provide a router object in the ApplicationConfig

I think ideally we would keep the 'simple' configuration with direct routes (using the routes-property, along side the advanced configuration using the router-key.

If possible, the Typescript definitions should indicate that only 1 configuration (routes vs router) is allowed at the same time

index.d.ts Outdated
Comment on lines 409 to 411
* Routes definition
*
* @example
Copy link
Collaborator

Choose a reason for hiding this comment

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

mention here as well that router takes precedence

Signed-off-by: Suresh Kumar Gangumalla <[email protected]>
1. Updated beforeAll to beforeEach
2. Added note that router config take precendence over routes

Signed-off-by: Suresh Kumar Gangumalla <[email protected]>
Updated Application type definition to accept only router or routes but
not both and Application configuration can still defined without both router and
routes as well.

Signed-off-by: Suresh Kumar Gangumalla <[email protected]>
@github-actions
Copy link

Test Results Summary: (Ran at: Thu Mar 13 09:43:57 UTC 2025)
passed: 491 failed: 0 of 491 tests (758ms)

Signed-off-by: Suresh Kumar Gangumalla <[email protected]>
@github-actions
Copy link

Test Results Summary: (Ran at: Thu Mar 13 10:30:55 UTC 2025)
passed: 491 failed: 0 of 491 tests (787ms)

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.

LGTM

@michielvandergeest michielvandergeest merged commit 54a9ac6 into lightning-js:dev Mar 13, 2025
2 checks passed
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