Skip to content

[APM] Typed client-side routing#104274

Merged
dgieselaar merged 25 commits intoelastic:masterfrom
dgieselaar:typed-router-config
Jul 15, 2021
Merged

[APM] Typed client-side routing#104274
dgieselaar merged 25 commits intoelastic:masterfrom
dgieselaar:typed-router-config

Conversation

@dgieselaar
Copy link
Copy Markdown
Contributor

@dgieselaar dgieselaar commented Jul 5, 2021

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:

image

Here's after, where there's almost no thread blocking:

image

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:

import { createRouter, Outlet, unconst } from '@kbn/typed-react-router-config';

const routes = unconst([
	{
		path: '/',
		element: <Outlet/>,
		children: [
			{
				path: '/services',
				element: <ServiceOverview/>,
				params: t.type({
					query: t.type({
						rangeFrom: t.string,
						rangeTo: t.string
					})
				})
			},
			{
				path: '/services/:serviceName',
				element: <Outlet/>,
				params: t.type({
					path: t.type({
						serviceName: t.string,
					}),
					query: t.type({
						rangeFrom: t.string
					})
				}),
				children: [
					{
						path: '/overview',
						element: <ServiceOverview/>
					},
					{
						path: '/transactions',
						element: <TransactionOverview/>
					}
				]
			}
		]
	}	
] as const);

const and unconst() are necessary to have exact types, without the overhead of having readonly arrays and objects.

Outlet is 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:

const apmRouter = createRouter(routes);

The router has the following methods:

  • link(routePath, params?): returns a link (a string) based on the routePath (which is the path defined in the route, concatenated with its parents). The type of params is 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

import { RouteRenderer, RouterProvider } from '@kbn/typed-react-router-config';
import { apmRouter } from './apm_route_config';

function App ( {}:{ history: History }) {
	
	return <RouterProvider router={apmRouter} history={history}>
		<MyCustomContextProvider>
			<RouteRenderer/>
		</MyCustomContextProvider>
	</RouterProvider>
}

RouterProvider renders a react-router context provider, to ensure its Route components still work, and its own context provider for the router passed to it.

RouteRenderer is the target for rendering the elements of active routes.

Hooks

  • useRouter(): returns the router in context

  • useParams(routePath): calls and returns router.getParams(routePath) for the router in context

  • useMatchRoutes(routePath?, location): calls and returns router.matchRoutes(routePath?, location) for the router in context

  • useApmParams(routePath): a typed version of useParams based on apmRouter

  • useApmRouter(): a typed version of useRouter based on useParams

Examples

TBD

@dgieselaar dgieselaar marked this pull request as ready for review July 8, 2021 17:36
@dgieselaar dgieselaar requested a review from a team July 8, 2021 17:36
@dgieselaar dgieselaar added release_note:skip Skip the PR/issue when compiling release notes v7.15.0 Team:APM - DEPRECATED Use Team:obs-ux-infra_services. labels Jul 9, 2021
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/apm-ui (Team:apm)

@cauemarcondes
Copy link
Copy Markdown
Contributor

cauemarcondes commented Jul 12, 2021

@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?

@dgieselaar
Copy link
Copy Markdown
Contributor Author

dgieselaar commented Jul 13, 2021

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.

Copy link
Copy Markdown
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL these exist. Some time soon I'll go and get us set up to use these automatically in Jest and Storybook.

@dgieselaar
Copy link
Copy Markdown
Contributor Author

@smith updated the docs and fixed the issue with the alert flyouts.

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1591 1559 -32

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
apm 4.4MB 4.3MB -115.0KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
apm 40.2KB 40.3KB +71.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@dgieselaar dgieselaar added the auto-backport Deprecated - use backport:version if exact versions are needed label Jul 15, 2021
@dgieselaar dgieselaar merged commit 821aeb1 into elastic:master Jul 15, 2021
@dgieselaar dgieselaar deleted the typed-router-config branch July 15, 2021 09:32
@kibanamachine
Copy link
Copy Markdown
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 104274

jloleysens added a commit to jloleysens/kibana that referenced this pull request Jul 15, 2021
…-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
dgieselaar added a commit to dgieselaar/kibana that referenced this pull request Jul 15, 2021
* [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:APM - DEPRECATED Use Team:obs-ux-infra_services. v7.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[APM] Improve client-side routes

5 participants