-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Add react-router-dom-v5-compat #159173
Conversation
…-ref HEAD~1..HEAD --fix'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ML changes LGTM
There was a problem hiding this 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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this 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 😄 ?
Thank you @pgayvallet! 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 |
@@ -5,6 +5,8 @@ | |||
* 2.0. | |||
*/ | |||
|
|||
// eslint-disable-next-line @kbn/eslint/module_migration | |||
import type { ExtractRouteParams } from 'react-router'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResponseOps changes LGTM
There was a problem hiding this 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
There was a problem hiding this 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 👍
There was a problem hiding this 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
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
## 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>
Summary
Prep work for bumping react-router to v6
Following remix-run/react-router#8753