Feat/current router #1819
Feat/current router #1819
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1819 +/- ##
==========================================
- Coverage 88.53% 88.41% -0.13%
==========================================
Files 248 250 +2
Lines 22290 22326 +36
Branches 5182 5181 -1
==========================================
+ Hits 19735 19740 +5
- Misses 2555 2586 +31 ☔ View full report in Codecov by Sentry. |
| snapshot: Partial<Navigation>; | ||
| }; | ||
|
|
||
| export const IRoute = DI.createInterface<IRoute>(); |
There was a problem hiding this comment.
we need to decorate this with /*@__PURE__*/ comment for tree shaking
| export const IRoute = DI.createInterface<IRoute>(); | |
| export const IRoute = /*@__PURE__*/ DI.createInterface<IRoute>(); |
|
|
||
| export const IRoute = DI.createInterface<IRoute>(); | ||
|
|
||
| export const RouteCallback = Registration.cachedCallback(IRoute, (container) => { |
There was a problem hiding this comment.
likely to be the same
| export const RouteCallback = Registration.cachedCallback(IRoute, (container) => { | |
| export const RouteCallback = /*@__PURE__*/ Registration.cachedCallback(IRoute, (container) => { |
| }, returnVal) ; | ||
| }; | ||
|
|
||
| export const RouteCallback = Registration.cachedCallback(IRoute, (container) => { |
There was a problem hiding this comment.
tree shaking related comment
| export const RouteCallback = Registration.cachedCallback(IRoute, (container) => { | |
| export const RouteCallback = /*@__PURE__*/ Registration.cachedCallback(IRoute, (container) => { |
| queryParams: TQueryParams; | ||
| snapshot: Partial<NavigationEndEvent>; | ||
| }; | ||
| export const IRoute = DI.createInterface<IRoute>(); |
There was a problem hiding this comment.
| export const IRoute = DI.createInterface<IRoute>(); | |
| export const IRoute = /*@__PURE__*/ DI.createInterface<IRoute>(); |
There was a problem hiding this comment.
@brandonseydel Thank you for this. Reviewed the router-lite part. Seems logical; however, as there are no tests, it is hard to verify that it works for different routing scenarios. Please consider writing tests.
| route.path = finalInstructions.toPath(); | ||
| route.url = finalInstructions.toUrl(true, router.options._urlParser); | ||
| route.title = localWindow.document.title; | ||
| route.queryParams = Array.from(queryParams).reduce<Record<string, string>>((acc, [key, value]) => { |
There was a problem hiding this comment.
This is not needed when queryParams is of type URLSearchParams.
| acc[key] = value; | ||
| return acc; | ||
| }, {}); | ||
| route.params = flattenParams(children); |
There was a problem hiding this comment.
Why flatten parameter? This results in loss of information.
| import { IRouter } from './router'; | ||
| import { Params, ViewportInstructionTree } from './instructions'; | ||
|
|
||
| export type IRoute<TParams = Record<string, string | undefined>, TQueryParams = Record<string, string | undefined>> = { |
| path: string; | ||
| url: string; | ||
| params: TParams; | ||
| queryParams: TQueryParams; |
There was a problem hiding this comment.
Should be of type URLSearchParams.
| url: string; | ||
| params: TParams; | ||
| queryParams: TQueryParams; | ||
| snapshot: Partial<NavigationEndEvent>; |
There was a problem hiding this comment.
I find the name snapshot to be very ambiguous. I would remove it completely, because semantically, the event cannot be a part of the current route. Furthermore, I would like to know the use-cases where this would be beneficial.
If kept, I would suggest removing the Partial, as why would the information provided by the framework be partial?
| title: string; | ||
| path: string; | ||
| url: string; | ||
| params: TParams; |
There was a problem hiding this comment.
An extra TParams is not needed, as there is already a Params type should for this very purpose.
Moreover, considering multi-level routing, this should be a map (for example, Map<component,Params>) of some sort.
| }; | ||
|
|
||
| export const RouteCallback = Registration.cachedCallback(IRoute, (container) => { | ||
| const route: IRoute = { |
There was a problem hiding this comment.
Instantiate the class instead.
Pull Request
📖 Description
🎫 Issues
👩💻 Reviewer Notes
📑 Test Plan
⏭ Next Steps