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

Add react-router-dom-v5-compat #159173

Merged

Conversation

patrykkopycinski
Copy link
Contributor

@patrykkopycinski patrykkopycinski commented Jun 7, 2023

Summary

Prep work for bumping react-router to v6
Following remix-run/react-router#8753

@patrykkopycinski patrykkopycinski changed the title Chore/react router dom v5 compat Add react-router-dom-v5-compat Jun 8, 2023
@patrykkopycinski patrykkopycinski marked this pull request as ready for review June 8, 2023 14:48
@patrykkopycinski patrykkopycinski requested review from a team as code owners June 8, 2023 14:48
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

Data Discovery changes LGTM 👍

Copy link
Contributor

@kc13greiner kc13greiner left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

Migrating to react-router 6 is a large scale effort, see the comments in #132600

I can only encourage such initiative, however, I think it would be very valuable to have a well defined plan and an high-level issue tracking the required work (unless we already have one, but I don't see it being mentioned in this PR). I'm scared of orphan PRs for that kind of work.

So, to clarify: what's the exact purpose of the PR / what are you trying to do / do we want to go further / what's the plan 😄 ?

@patrykkopycinski
Copy link
Contributor Author

Thank you @pgayvallet!
The idea is, of course, to lead us to react-router@6, but as you could have experienced that by yourself, it's not an easy task and it requires middle steps, so that is why I have decided to follow the Official v5 to v6 Migration Guide
I'm working with @tkajtoch on the EUI-side to sync the effort between Kibana and EUI

After this PR gets merged I'm going to work with the Teams to either sync the effort of upgrading to v6 components or will create a PR and ask for the 🟢

Regarding react-router-config package, now it's part of react-router@6, as useRoutes, so we should be able to migrate this part easily :)

@@ -5,6 +5,8 @@
* 2.0.
*/

// eslint-disable-next-line @kbn/eslint/module_migration
import type { ExtractRouteParams } from 'react-router';
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think react-router-dom exports the same type.

Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

ResponseOps changes LGTM

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Compat router additions in Presentation team code LGTM! Code only review

Copy link
Contributor

@claracruz claracruz left a comment

Choose a reason for hiding this comment

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

Platform Onboarding changes LGTM 👍

@patrykkopycinski patrykkopycinski added the release_note:skip Skip the PR/issue when compiling release notes label Jun 13, 2023
@patrykkopycinski patrykkopycinski enabled auto-merge (squash) June 13, 2023 20:40
Copy link
Contributor

@gbamparop gbamparop left a comment

Choose a reason for hiding this comment

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

APM changes LGTM!

…ter-dom-v5-compat

# Conflicts:
#	packages/core/chrome/core-chrome-browser-internal/src/ui/project/header.tsx
@patrykkopycinski patrykkopycinski merged commit 09577fa into elastic:main Jun 14, 2023
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Threat Intelligence Tests #2 / Invalid Indicators verify the grid loads even with missing fields should display data grid despite the missing fields
  • [job] [logs] Security Solution Tests #4 / Prebuilt rules Actions with prebuilt rules Rules table "before each" hook for "Allows to delete all rules at once"

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/ui-shared-deps-src 36 37 +1

Async chunks

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

id before after diff
advancedSettings 50.7KB 50.8KB +43.0B
apm 3.6MB 3.6MB +54.0B
core 145.5KB 145.5KB +5.0B
dashboard 358.6KB 358.6KB +49.0B
dataViewManagement 118.7KB 118.7KB +45.0B
devTools 4.8KB 4.8KB +43.0B
discover 418.0KB 418.1KB +47.0B
profiling 290.4KB 290.4KB +50.0B
upgradeAssistant 152.3KB 152.3KB +46.0B
ux 165.4KB 165.4KB +50.0B
watcher 162.7KB 162.8KB +45.0B
total +477.0B

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.3KB +66.0B
apm 33.4KB 33.5KB +66.0B
cases 153.0KB 153.0KB -4.0B
core 369.3KB 369.6KB +262.0B
dashboard 39.3KB 39.4KB +66.0B
dataViewManagement 4.6KB 4.6KB +66.0B
devTools 10.9KB 10.9KB +66.0B
discover 27.5KB 27.5KB +66.0B
kbnUiSharedDeps-npmDll 6.0MB 6.1MB +85.9KB
kbnUiSharedDeps-srcJs 2.2MB 2.2MB +103.0B
profiling 14.4KB 14.5KB +66.0B
upgradeAssistant 19.4KB 19.5KB +66.0B
ux 6.4KB 6.5KB +66.0B
watcher 13.5KB 13.5KB +66.0B
total +86.9KB
Unknown metric groups

API count

id before after diff
@kbn/ui-shared-deps-src 45 46 +1

ESLint disabled line counts

id before after diff
cases 54 55 +1
enterpriseSearch 13 15 +2
securitySolution 409 413 +4
total +7

Total ESLint disabled count

id before after diff
cases 71 72 +1
enterpriseSearch 14 16 +2
securitySolution 492 496 +4
total +7

History

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

@kibanamachine kibanamachine added v8.9.0 backport:skip This commit does not require backporting labels Jun 14, 2023
jbudz pushed a commit that referenced this pull request Jun 23, 2023
## Summary

Why?

To simplify the process of migration to react-router@6.
remix-run/react-router#8753

What problems exactly it solves?

- In my previous PR I added `CompatRouter`
#159173, which caused changes in
~50 files and pinged 15 Teams. And this is just meant to be a temporary
change, so when we're done with the migration I would have to revert
these changes and engage everyone to review the PR again. And it is just
a single step in the migration strategy. So to make our lives easier I
think it would be better to have a common place where we do import our
router components because it will allow us to surface some extra logic
in single place instead of going through the whole source code again.

- `react-router@6` doesn't support a custom `Route` component, so that
means our custom `Route` component that we're using almost everywhere
today, will need to be replaced by a different solution. I have decided
to add `Routes` component, which will be responsible for rendering the
proper component (`react-router@6` renamed `Switch` to `Routes`, so I
have named this component to align with the dictionary of the new
router) and also is going to add the logic that today is done in `Route`
(moving logic to `Routes` will be done in the follow-up PR, here I just
wanted to focus on using the common router components to make the review
process easier)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
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 ci:all-cypress-suites release_note:skip Skip the PR/issue when compiling release notes Team:APM All issues that need APM UI Team support Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.