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] Add deprecation comments to router components to encourage usage of the shared-ux-router-component #150340

Closed
wants to merge 2 commits into from

Conversation

rshen91
Copy link
Contributor

@rshen91 rshen91 commented Feb 6, 2023

Summary

This PR adds comments to alert Kibana developers on deprecation of router components that are not the refactored (#138544) "@kbn/shared-ux-router" package.

Router is being used currently from either the kibana_react implementation or from react-router-dom

Initial Findings

  • There are over 900 instances of imports react-router-dom currently in Kibana. Not all these import statements are importing Router, but sometimes useHistory, withRouter, RouteComponentProps etc.
  • Instances of kibana_react's Router that need to be migrated should show the deprecation comment after this PR is merged.

If issues are found with the @kbn/shared-ux-router, please open a new bug issue for us to improve it. Thanks!

@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 marked this pull request as ready for review February 7, 2023 17:25
@rshen91 rshen91 requested a review from a team as a code owner February 7, 2023 17:25
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id before after diff
cases 104 115 +11
securitySolution 357 538 +181
total +192

History

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

cc @rshen91

@rshen91 rshen91 closed this Feb 13, 2023
mistic added a commit that referenced this pull request Feb 14, 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:

> 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:
#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.

- [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 <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Tiago Costa <tiagoffcc@hotmail.com>
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 <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Tiago Costa <tiagoffcc@hotmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants