Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[APM] Typed client-side routing #104274

Merged
merged 25 commits into from
Jul 15, 2021
Merged

Conversation

dgieselaar
Copy link
Member

@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 as a code owner July 8, 2021 17:36
@dgieselaar dgieselaar added release_note:skip Skip the PR/issue when compiling release notes v7.15.0 Team:APM All issues that need APM UI Team support labels Jul 9, 2021
@elasticmachine
Copy link
Contributor

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

@cauemarcondes
Copy link
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
Member 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
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:

search: fromQuery(mockParams),
};

const uiSettings = uiSettingsServiceMock.create().setup({} as any);
Copy link
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.

x-pack/plugins/apm/public/components/routing/app_root.tsx Outdated Show resolved Hide resolved
x-pack/plugins/apm/public/context/breadcrumbs/context.tsx Outdated Show resolved Hide resolved
@dgieselaar
Copy link
Member Author

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

@kibanamachine
Copy link
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
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 All issues that need APM UI Team support v7.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[APM] Improve client-side routes
5 participants