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] Support multiple route paths in useApmParams #109370

Merged
merged 2 commits into from
Aug 23, 2021

Conversation

dgieselaar
Copy link
Member

Summary

Support multiple route paths in useApmParams, which is useful when a component is expected to be rendered on multiple pages:

const { path, query } = useApmParams('/service-map', '/services/:serviceName/service-map');

@dgieselaar dgieselaar added Team:APM All issues that need APM UI Team support release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.16.0 labels Aug 20, 2021
@dgieselaar dgieselaar requested a review from a team as a code owner August 20, 2021 07:45
@elasticmachine
Copy link
Contributor

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

@sorenlouv
Copy link
Member

sorenlouv commented Aug 20, 2021

When supplying multiple routes to useApmParams will it narrow the params to only the intersection (commonalities)?

My gut feeling is that this will be more confusing than useful. For re-usable components that's used across different routes, I think the "react-way" is to pass the params as props.

@dgieselaar
Copy link
Member Author

@sqren it will be a union type. My first guess was also to pass these props down from components that are guaranteed to be on one route, but it's pretty annoying to pass upwards of 10 query parameters down multiple components, especially when some of those parameters are optional, making it easy to forget about them.

@sorenlouv
Copy link
Member

sorenlouv commented Aug 20, 2021

My first guess was also to pass these props down from components that are guaranteed to be on one route, but it's pretty annoying to pass upwards of 10 query parameters down multiple components, especially when some of those parameters are optional, making it easy to forget about them.

Yeah, I can see that. Was also the primary reason to create this hook in the first place.

it will be a union type.

I think we mean the same thing. I just always confuse intersection and union types.

Btw. It's probably just me (it most certainly is...) but it feels like TS reversed the meaning of union and intersection. In math union is represented by the upper image and intersection is represented by the bottom image:
image

@dgieselaar
Copy link
Member Author

@elasticmachine merge upstream

@@ -10,6 +10,7 @@ import '../../../typings/rison_node';
import '../../infra/types/eui';
// EUIBasicTable
import '../../reporting/public/components/report_listing';
import '../../reporting/server/lib/puid';
Copy link
Member Author

Choose a reason for hiding this comment

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

this is needed for the typescript optimisation script

@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Metrics [docs]

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.4MB +1.3KB

History

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

@dgieselaar dgieselaar merged commit 6e3af2b into elastic:master Aug 23, 2021
@dgieselaar dgieselaar deleted the apm-params-multiple-paths branch August 23, 2021 13:38
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 23, 2021
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 23, 2021
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.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants