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

Remove unused type defs and packages #1682

Merged
merged 2 commits into from
Aug 15, 2023

Conversation

anshgoyalevil
Copy link
Member

@anshgoyalevil anshgoyalevil commented Aug 15, 2023

Related to: #1675 and #1672

What changes is this PR making?

  • This PR removes the unused packages from packages/jaeger-ui
  • The removed packages are:
    • @svgr/babel-plugin-transform-svg-component
    • @svgr/babel-preset
    • @types/react-dom
    • @types/react-router-redux

Description of the changes

Previously, I was reluctant in removing @types/react-dom and @types/react-router-redux because their corresponding packages are used, but I gave them a test by removing:

  • run yarn lint
  • no errors
  • remove these two packages and again run yarn lint
  • no errors
  • maybe linter is not working correctly, so removed @types/react-window
  • run yarn lint
  • type errors
  • added back @types/react-window and run yarn lint
  • no errors
  • Thus, these two deps aren't needed

How was this change tested?

  • By running yarn lint and yarn test

Checklist

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (1e20f47) 96.01% compared to head (5135b47) 96.01%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1682   +/-   ##
=======================================
  Coverage   96.01%   96.01%           
=======================================
  Files         241      241           
  Lines        7560     7560           
  Branches     1985     1985           
=======================================
  Hits         7259     7259           
  Misses        301      301           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anshgoyalevil anshgoyalevil changed the title Remove unused svgr packages Remove unused type defs and packages Aug 15, 2023
@yurishkuro yurishkuro merged commit 82344f3 into jaegertracing:main Aug 15, 2023
anshgoyalevil added a commit to anshgoyalevil/jaeger-ui that referenced this pull request Aug 16, 2023
Related to: jaegertracing#1675 and
jaegertracing#1672
<!--
!! Please DELETE this comment before posting.
We appreciate your contribution to the Jaeger project! 👋🎉
-->

## What changes is this PR making?
- This PR removes the unused packages from packages/jaeger-ui
- The removed packages are:
   - `@svgr/babel-plugin-transform-svg-component`
   - `@svgr/babel-preset`
   - `@types/react-dom`
   - `@types/react-router-redux`

## Description of the changes
- As per the discussion
jaegertracing#1226 (comment),
and the manual check.

Previously, I was reluctant in removing `@types/react-dom` and
`@types/react-router-redux` because their corresponding packages are
used, but I gave them a test by removing:
- run `yarn lint`
- no errors
- remove these two packages and again run `yarn lint`
- no errors
- maybe linter is not working correctly, so removed
`@types/react-window`
- run `yarn lint`
- type errors
- added back `@types/react-window` and run `yarn lint`
- no errors
- Thus, these two deps aren't needed

## How was this change tested?
- By running yarn lint and yarn test

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [x] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Ansh Goyal <[email protected]>
@anshgoyalevil anshgoyalevil deleted the rm-svgr branch August 16, 2023 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants