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

[Shared UX] Adoption of Shared UX Route component #150357

Merged
merged 41 commits into from
Feb 14, 2023

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Feb 6, 2023

Summary

This PR removes all imports of Route from react-router-dom and '@kbn/kibana-react-plugin/public' and instead imports Route from @kbn/shared-ux-router.

Context

Based on #132629 (comment) This PR executes steps 2 - 4:

  1. To make the transition easier, we want to re-export other react-router-dom exports alongside the modified' Route'.
  2. Solutions should start using that Route component in place of the one from react-router-dom. I.e. replace all occurrences of import { ... } from 'react-router-dom' with import { ... } from '@kbn/shared-ux-router'.
  3. All manual calls to useExecutionContext are not needed anymore and should be removed.

Future PR

Looks like this might be getting worked on in: #145863 (thanks!)

Introduce an ESlint rule that ensures that react-router-dom is not used directly in Kibana and that imports go through the new @kbn/shared-ux-router package.

This is tangentially accomplished through #150340 but only addresses using Route through @kbn/kibana-react-plugin/public'

Checklist

Delete any items that are not applicable to this PR.

@rshen91 rshen91 changed the title [Shared UX] Adoption of shared ux Route component [Shared UX] Adoption of hared ux Route component Feb 6, 2023
@rshen91 rshen91 changed the title [Shared UX] Adoption of hared ux Route component [Shared UX] Adoption of Shared UX Route component Feb 6, 2023
@rshen91 rshen91 self-assigned this Feb 6, 2023
@rshen91 rshen91 added the release_note:skip Skip the PR/issue when compiling release notes label Feb 6, 2023
@rshen91 rshen91 requested a review from xcrzx February 6, 2023 18:46
@rshen91 rshen91 marked this pull request as ready for review February 7, 2023 21:34
@rshen91 rshen91 requested review from a team as code owners February 7, 2023 21:34
@rshen91 rshen91 requested a review from a team February 7, 2023 21:34
@rshen91 rshen91 requested review from a team as code owners February 7, 2023 21:34
@rshen91 rshen91 requested a review from a team as a code owner February 13, 2023 17:56
Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM for Core owned changes

Copy link
Contributor

@juliaElastic juliaElastic left a comment

Choose a reason for hiding this comment

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

Fleet changes LGTM

Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all your work on this @rshen91 ❤️

.eslintrc.js Show resolved Hide resolved
Co-authored-by: Tiago Costa <[email protected]>
@mistic mistic disabled auto-merge February 14, 2023 19:24
@mistic mistic merged commit 50a4fc4 into elastic:main Feb 14, 2023
@kibanamachine kibanamachine added v8.8.0 backport:skip This commit does not require backporting labels Feb 14, 2023
@rshen91 rshen91 deleted the migrate-to-shared-ux-router branch February 14, 2023 19:26
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
advancedSettings 46 54 +8
apm 1388 1394 +6
canvas 1097 1105 +8
cases 597 601 +4
cloudSecurityPosture 134 140 +6
crossClusterReplication 139 147 +8
dashboard 354 362 +8
dataViewManagement 122 130 +8
devTools 17 25 +8
discover 435 443 +8
enterpriseSearch 1948 1954 +6
filesManagement 125 133 +8
fleet 773 779 +6
graph 260 268 +8
indexLifecycleManagement 185 193 +8
indexManagement 480 488 +8
infra 1251 1255 +4
ingestPipelines 322 330 +8
kibanaOverview 107 115 +8
kubernetesSecurity 53 59 +6
lens 891 895 +4
licenseManagement 129 137 +8
logstash 54 62 +8
management 98 106 +8
maps 963 971 +8
ml 1709 1715 +6
monitoring 448 456 +8
observability 515 521 +6
osquery 257 265 +8
savedObjectsManagement 78 86 +8
security 491 499 +8
securitySolution 3716 3722 +6
snapshotRestore 196 204 +8
spaces 251 259 +8
synthetics 1355 1359 +4
transform 299 307 +8
triggersActionsUi 510 518 +8
upgradeAssistant 130 138 +8
visualizations 326 332 +6
watcher 177 185 +8
total +286

Async chunks

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

id before after diff
advancedSettings 48.2KB 50.5KB +2.3KB
apm 3.3MB 3.3MB +1.2KB
canvas 1023.6KB 1.0MB +2.3KB
cases 394.2KB 395.0KB +857.0B
cloudSecurityPosture 142.9KB 144.0KB +1.2KB
crossClusterReplication 166.5KB 168.2KB +1.6KB
dashboard 379.6KB 382.0KB +2.4KB
dataViewManagement 116.3KB 118.6KB +2.3KB
devTools 2.5KB 4.8KB +2.3KB
discover 392.3KB 394.7KB +2.3KB
enterpriseSearch 2.1MB 2.1MB +950.0B
filesManagement 83.6KB 86.0KB +2.4KB
fleet 928.2KB 929.3KB +1.2KB
graph 450.8KB 453.2KB +2.4KB
indexLifecycleManagement 156.2KB 157.8KB +1.6KB
indexManagement 522.4KB 524.6KB +2.2KB
infra 1.3MB 1.3MB +2.2KB
ingestPipelines 433.3KB 435.5KB +2.3KB
kibanaOverview 40.8KB 43.2KB +2.4KB
kubernetesSecurity 39.6KB 40.7KB +1.2KB
lens 1.3MB 1.3MB +790.0B
licenseManagement 62.9KB 65.2KB +2.4KB
logstash 30.6KB 33.1KB +2.4KB
management 31.6KB 34.0KB +2.4KB
maps 2.7MB 2.7MB +2.4KB
ml 3.5MB 3.5MB +1.2KB
monitoring 455.4KB 457.8KB +2.4KB
observability 585.0KB 586.3KB +1.2KB
osquery 1.1MB 1.1MB +2.3KB
savedObjectsManagement 85.2KB 87.6KB +2.4KB
security 557.3KB 557.3KB +7.0B
securitySolution 13.8MB 13.8MB +914.0B
snapshotRestore 267.4KB 269.6KB +2.2KB
spaces 174.4KB 174.4KB +4.0B
synthetics 1.4MB 1.4MB +964.0B
transform 366.4KB 368.8KB +2.4KB
triggersActionsUi 901.4KB 906.3KB +5.0KB
upgradeAssistant 149.9KB 152.2KB +2.4KB
visualizations 258.5KB 259.6KB +1.2KB
watcher 271.6KB 273.3KB +1.7KB
total +74.1KB

Page load bundle

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

id before after diff
advancedSettings 10.2KB 10.2KB +49.0B
crossClusterReplication 11.9KB 11.9KB +49.0B
devTools 10.7KB 10.7KB +49.0B
indexLifecycleManagement 26.9KB 27.0KB +49.0B
indexManagement 27.1KB 27.1KB +49.0B
ingestPipelines 13.5KB 13.5KB -5.0B
licenseManagement 10.2KB 10.2KB +50.0B
savedObjectsManagement 19.3KB 19.4KB +49.0B
security 57.9KB 60.4KB +2.5KB
spaces 20.4KB 22.8KB +2.3KB
upgradeAssistant 19.4KB 19.4KB +49.0B
watcher 13.4KB 13.4KB +49.0B
total +5.2KB
Unknown metric groups

ESLint disabled line counts

id before after diff
@kbn/core-application-browser-internal 1 2 +1
@kbn/shared-ux-router 0 1 +1
crossClusterReplication 7 8 +1
home 9 10 +1
indexManagement 14 16 +2
kibanaReact 8 9 +1
remoteClusters 2 3 +1
rollup 5 6 +1
synthetics 91 93 +2
total +11

Total ESLint disabled count

id before after diff
@kbn/core-application-browser-internal 1 2 +1
@kbn/shared-ux-router 0 1 +1
crossClusterReplication 9 10 +1
home 10 11 +1
indexManagement 18 20 +2
kibanaReact 12 13 +1
remoteClusters 2 3 +1
rollup 5 6 +1
synthetics 97 99 +2
total +11

History

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

cc @rshen91

justinkambic pushed a commit to justinkambic/kibana that referenced this pull request Feb 23, 2023
## Summary

This PR removes all imports of Route from react-router-dom and
'@kbn/kibana-react-plugin/public' and instead imports Route from
@kbn/shared-ux-router.

### Context
Based on
elastic#132629 (comment) This PR
executes steps 2 - 4:

> 2. To make the transition easier, we want to re-export other
react-router-dom exports alongside the modified' Route'.
> 3. Solutions should start using that Route component in place of the
one from react-router-dom. I.e. replace all occurrences of import { ...
} from 'react-router-dom' with import { ... } from
'@kbn/shared-ux-router'.
> 4. All manual calls to useExecutionContext are not needed anymore and
should be removed.

### Future PR

Looks like this might be getting worked on in:
elastic#145863 (thanks!)

> Introduce an ESlint rule that ensures that react-router-dom is not
used directly in Kibana and that imports go through the new
@kbn/shared-ux-router package.

This is tangentially accomplished through
elastic#150340 but only addresses using
Route through @kbn/kibana-react-plugin/public'


### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Tiago Costa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Fleet Team label for Observability Data Collection Fleet team Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.