feat(devtools): clean up router tree for stable release #63081
Conversation
7af49a9 to
0be47a7
Compare
| if (router && router.navigateByUrl) { | ||
| router.navigateByUrl(path); | ||
| if (router) { | ||
| (ngDebugClient() as any).ɵnavigateByUrl?.(router, path); |
There was a problem hiding this comment.
Consider: I remember bringing this up before, but is it possible to depend on the ng global functions exposed by router such that we don't need these as any casts?
There was a problem hiding this comment.
Not sure why this was the case. Theres a comment on the interface about an issue with the API extractor. I've attempted a refactor to make this type aware, lets see if the tests pass in CI
| export function navigateByUrl(router: Router, url: string): Promise<boolean> { | ||
| if (!(router instanceof Router)) { | ||
| throw new Error('The provided router is not an Angular Router.'); | ||
| } |
There was a problem hiding this comment.
Consider: Is there a particular reason to check for Router instance? Do we have reason to believe this is likely to be the wrong type? We don't check Injector or Route above (in fairness, I think those are interfaces rather than classes, but we certainly could check properties on them if we really wanted to).
I'm wondering if we're losing much by just omitting this check.
There was a problem hiding this comment.
This is just defensive programming, it would be very unexpected to hit this case
| import {TestBed} from '@angular/core/testing'; | ||
| import {Router, RouterModule} from '../index'; | ||
| import {getLoadedRoutes} from '../src/router_devtools'; | ||
| import {getLoadedRoutes, getRouterInstance, navigateByUrl} from '../src/router_devtools'; |
There was a problem hiding this comment.
Consider: Could we improve this by testing the published ng global rather than directly importing the symbols? What if we called provideRouter and then called ng.* and validated that result?
devtools/projects/ng-devtools/src/lib/devtools-tabs/router-tree/route-details-row.component.ts
Outdated
Show resolved
Hide resolved
devtools/projects/ng-devtools/src/lib/devtools-tabs/router-tree/route-details-row.component.ts
Outdated
Show resolved
Hide resolved
devtools/projects/ng-devtools/src/lib/devtools-tabs/router-tree/route-details-row.component.ts
Show resolved
Hide resolved
b980826 to
92d9188
Compare
Addresses some cleanup items for the router tree: - No longer loads router ng global APIs as a side effect of importing the router. Rather this is now a runtime step that occurs when provideRouter is called. - No longer depends on router.navigateByUrl in Angular DevTools. There is now a dedicated global util for this - Router instance logic no longer depends on token name - Prevents navigating to lazy or redirect routes (these don't have an associated component)
92d9188 to
fe2d6e1
Compare
|
@AleksanderBodurri just a heads up that this PR is having all sorts of github / pullapprove issues. I can't remove Paul's review request or my own. Pullapprove is erroring. So for now just leave the |
Addresses some cleanup items for the router tree: