[APM] Typed client-side routing#104274
Conversation
|
Pinging @elastic/apm-ui (Team:apm) |
|
@dgieselaar I see you defined the query params as required in one of the examples: params: t.type({
query: t.type({
rangeFrom: t.string,
rangeTo: t.string
})
})should we make it all optional since it comes from the URL and the user could manually remove it? |
They're required in the example, but optional in the application code. Preferably we make them required there as well, and figure out a way to set defaults on the route definition. If we make them optional everywhere, it's easy to forget passing them on. But let's address that in a follow-up PR. |
smith
left a comment
There was a problem hiding this comment.
I truly love this.
I had some comments on the PR and a few things:
- Update the Routing and Linking internal docs
- When I'm on the service inventory and try to create any kind of alert there's an error about the routing
- The storybook for the latency chart needs to be updated to work with the new router. I can fix this in #104991 so that's not a blocker
| search: fromQuery(mockParams), | ||
| }; | ||
|
|
||
| const uiSettings = uiSettingsServiceMock.create().setup({} as any); |
There was a problem hiding this comment.
TIL these exist. Some time soon I'll go and get us set up to use these automatically in Jest and Storybook.
|
@smith updated the docs and fixed the issue with the alert flyouts. |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
💔 Backport failed
To backport manually run: |
…-png-pdf-report-type * 'master' of github.com:elastic/kibana: (75 commits) [Search Sessions] Don’t try to delete errored searches (elastic#105434) [Search Sessions] fix saved object can be created even if courier:batchSearches is enabled (elastic#105407) [Remote Clusters] Fixed remote clusters details flyout for long strings (elastic#105592) [ML] Functional tests - re-activate a11y tests (elastic#105198) [APM] Typed client-side routing (elastic#104274) [Canvas] Expression error (elastic#103048) [ML] Fixing job wizard with missing description (elastic#105574) [Security Solution][Alerts] - Add alerts subfeature UI (elastic#105505) Upgrade EUI to v35.0.0 (elastic#105127) [Reporting] Clean up types for internal APIs needed for UI (elastic#105508) skip flaky suite (elastic#105087) [Workplace Search] Fix Chrome issues with GitHub sources (elastic#105680) [Fleet] Add containerized fleet server instructions to Fleet README (elastic#105669) [ML] Add api integration test for analytics map endpoint (elastic#105531) Fixes cypress flake across two tests (elastic#105645) [Logs&Metrics UI] add owner properties to plugin manifest (elastic#105580) chore(NA): introduce preset for jest-integration tests on @kbn/test (elastic#105144) [Enterprise Search] Added Thumbnails to Search UI (elastic#104199) Translate App Search credentials list (elastic#105619) [APM] APM agent config created prior to Fleet migration is not injected into integration policy (elastic#105504) ... # Conflicts: # x-pack/plugins/reporting/common/types.ts # x-pack/plugins/reporting/public/management/report_listing.test.tsx
* [APM] @kbn/typed-router-config * [APM] typed route config * Breadcrumbs, wildcards * Migrate settings, home * Migrate part of service detail page * Migrate remaining routes, tests * Set maxWorkers for precommit script to 4 * Add jest types to tsconfigs * Make sure transaction distribution data is fetched * Fix typescript errors * Remove usage of react-router's useParams * Add route() utility function * Don't use ApmServiceContext for alert flyouts * Don't add onClick handler for breadcrumb * Clarify ts-ignore * Remove unused things * Update documentation * Use useServiceName() in ServiceMap component
Closes #51963.
Whatever you do, don't look at the type definitions.
This PR adds typed client-side routing, by inferring route/path names and their expected path and query parameters.
Here's a demo:
Screen.Recording.2021-07-08.at.20.23.11.mov
I've profiled some tab switches in the service overview as well. Looks like a lot less stuff happens after this change. Here's the before, where you can see the main thread block for about 600-800ms on every tab switch:
Here's after, where there's almost no thread blocking:
I assume this is because there's less mounting/unmounting happening.
It works as follows:
Defining routes
Routes are defined according to the proposal for object-based routing in
react-router@6:constandunconst()are necessary to have exact types, without the overhead of having readonly arrays and objects.Outletis a component that will render the element of an active route's child element.Creating a router
The route definition should then be passed to
createRouter:The router has the following methods:
link(routePath, params?): returns a link (a string) based on the routePath (which is thepathdefined in the route, concatenated with its parents). The type ofparamsis based on the found route definition (and its parents), and is guarded by a runtime validation as well.matchRoutes(routePath?, location): returns matching routes up until the routePath, if it matches, or all routes if a wildcard is used. If only the location is given, it will return all matching routes.getParams(routePath?, location): similar to matchRoutes, but will validate, parse and return all parameters for the matched routes.Rendering the routes
RouterProviderrenders areact-routercontext provider, to ensure itsRoutecomponents still work, and its own context provider for the router passed to it.RouteRendereris the target for rendering the elements of active routes.Hooks
useRouter(): returns the router in contextuseParams(routePath): calls and returnsrouter.getParams(routePath)for the router in contextuseMatchRoutes(routePath?, location): calls and returnsrouter.matchRoutes(routePath?, location)for the router in contextuseApmParams(routePath): a typed version ofuseParamsbased onapmRouteruseApmRouter(): a typed version ofuseRouterbased onuseParamsExamples
TBD