Removes APM custom breadcrumbs, leaves only chrome.breadcrumbs.set #29286
Conversation
| type BreadcrumbFunction = (args: BreadcrumbArgs) => string | null; | ||
|
|
||
| interface Route extends RouteProps { | ||
| switch?: never; |
There was a problem hiding this comment.
i'm not sure this is the intention of the never type. should it be boolean instead?
There was a problem hiding this comment.
This key should "never" be present on this type. This is the end result of a ton of messing around with discriminated union stuff, we can chat...
There was a problem hiding this comment.
You can simplify this if you combine the switch and routes properties into just a switchRoutes prop (renamed for clarity). Then you can type it like:
interface Route extends RouteProps {
switchRoutes?: Route[];
breadcrumb?: string | BreadcrumbFunction | null;
}
type RoutesConfig = Route[];If you want the union you can do this instead:
interface NormalRoute extends RouteProps {
breadcrumb: string | BreadcrumbFunction | null;
}
interface SwitchRoute extends RouteProps {
switchRoutes: Route[];
}
type Route = SwitchRoute | NormalRoute;
type RoutesConfig = Route[];But I think the extra type narrowing is not really warranted.
There was a problem hiding this comment.
The first one works, but I have to then be careful to update where we build out the routes to use the new key, and I was really hoping TS would allow me to keep it working the way it was already working. The second example here doesn't seem to work without the other stuff I added to tell TS about which type a route is.
If we're ok with changing the key to switchRoutes and just checking for its existence, I'm good with that.
There was a problem hiding this comment.
If you prefer the second solution, we can try working towards that. What do you mean "it doesn't work"?
If we're ok with changing the key to switchRoutes and just checking for its existence, I'm good with that.
Yes, I think this is the better solution, Typescript or not. Just didn't think about it back when I initially wrote it
There was a problem hiding this comment.
I don't really care that much. :) I made the change to switchRoutes 👍
| UpdateBreadcrumbsComponent | ||
| ); | ||
|
|
||
| export { UpdateBreadcrumbs }; |
There was a problem hiding this comment.
It's strange that this component's only purpose is its side effects. i feel like this is a case where store.subscribe is useful.
There was a problem hiding this comment.
Agreed that it's not the best. I thought about looking into changing it but I see we already do this with Main/LicenseChecker, so I left it alone.
There was a problem hiding this comment.
I'm fine with how you did it here - but if there is a better approach I think we should take that, and update LicenseChecker accordingly in another PR.
There was a problem hiding this comment.
TBH I'm not sure if there is a way that's significantly better, sometimes at the top-level you just have some side-effect components (we definitely had these at my last job), and personally I'm fine with that if they're contained up there. If folks have a specific better idea and want to log something, go for it.
💔 Build Failed |
💔 Build Failed |
|
retest |
💔 Build Failed |
jasonrhodes
force-pushed
the
remove-apm-breadcrumb
branch
from
b753b7b to
ea69413
Compare
💚 Build Succeeded |
💚 Build Succeeded |
Closes #25906
Summary
After much discussion in the comments of #25906, we decided to leave the APM breadcrumbs working the way they are for now until we can consider how to update navigation elements after K7 lands. In this PR I've done the following:
showPluginBreadcrumbs(this checked if K7 theme was on, which will always be on in 7.0+)<Breadcrumbs>component to<UpdateBreadcrumbs>as it is only now responsible for callingchrome.breadcrumbs.set()when the breadcrumb changes.