Fix Console Router invalid param warnings #16320
Conversation
…t have a resource param
abab37e to
41865e9
Compare
| descending: { | ||
| type: Boolean, | ||
| required: true | ||
| required: false, |
There was a problem hiding this comment.
Unrelated but there was another warning where we were passing undefined to this. Since there wasn't any related bugs that I found I think it's safe to default to false.
41865e9 to
24d7622
Compare
| export function findRouteDefinitionByName(router, routeName) { | ||
| const routes = router.getRoutes(); | ||
|
|
||
| return routes.find((r) => r.name === routeName); | ||
| } |
There was a problem hiding this comment.
I did this in the simplest programmatic way. I didn't find any performance issues so I left it but it would be possible to cashing these in a name-to-route object. We'd have to be careful with extensions which I wanted to avoid unless it becomes necessary.
rak-phillip
left a comment
There was a problem hiding this comment.
Testing these changes and everything is looking good. The warnings appear to be resolved across the board. I only have one question before we merge.
|
|
||
| // Having an undefined param can yield a console warning like [Vue Router warn]: Discarded invalid param(s) "namespace" when navigating | ||
| if (location && !location.params.namespace) { | ||
| delete location.params.namespace; | ||
| } | ||
|
|
||
| return location; |
There was a problem hiding this comment.
If I understand, the lack of access to a valid router is what prevents us from using the new filterLocationValidParams() in locations like this, RelatedResource, and LinkName so the manual deletion is needed in these areas. Is that correct?
There was a problem hiding this comment.
So that would be a reasonable reason for this change but it wasn't the motivation for my choice. In this scenario and the other like it, we know what route we're navigating to and we can adjust the params without having to do a lookup.
If you'd like to be consistent I can look into plumping the router in.
There was a problem hiding this comment.
Hmm.. While the consistency might be nice, I don't think it will be worth it if there's a level of effort involved. I think this is good as it is.
There was a problem hiding this comment.
Hmm.. While the consistency might be nice, I don't think it will be worth it if there's a level of effort involved. I think this is good as it is.
Summary
Fixes #12128
Fixes #16220
Occurred changes and/or fixed issues
We now check to make sure params are present on a route definition before passing it to any $router methods.
Technical notes summary
Sometimes we look up the route definition and sometimes we conditionally send a param if we know they're available.
Areas or cases that should be tested
Any management, global or explorer page should be checked. This was primarily an issue with out navigation sections.
Areas which could experience regressions
Any management, global or explorer page should be checked. This was primarily an issue with out navigation sections.
Checklist
Admin,Standard UserandUser Base