Remove unused packages #1675
Merged
Remove unused packages #1675
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Ansh Goyal <[email protected]>
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## main #1675 +/- ##
=======================================
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. |
yurishkuro
reviewed
Aug 14, 2023
packages/jaeger-ui/package.json
Outdated
| @@ -15,8 +15,6 @@ | |||
| "@babel/preset-env": "^7.20.2", | |||
| "@babel/preset-react": "^7.18.6", | |||
| "@babel/preset-typescript": "^7.21.0", | |||
| "@svgr/babel-plugin-transform-svg-component": "^8.0.0", | |||
| "@svgr/babel-preset": "^8.0.0", | |||
There was a problem hiding this comment.
how do we know these are not used by babel toolchain?
There was a problem hiding this comment.
According to their NPM Readme's, they need to be included inside the .babelrc or in our case, babel-transform.js file.
There was a problem hiding this comment.
Also, according to its implementation guide, it can only be used after defining the @svgr/{plugin-name} in some file (For example, webpack, cli, node, nextjs), which I couldn't find anywhere inside the codebase.
There was a problem hiding this comment.
can you look back in git history when they were added and why? were they used in the past?
There was a problem hiding this comment.
I checked it in the git history. Here's it is https://github.com/jaegertracing/jaeger-ui/pull/1226/files#diff-7efdc547c40e0d5e99b6ecbdba5ac9ae13de8f3d2b1ca15332363868462176e2R18-R19
It was at that time also not used anywhere.
4 tasks
|
@anshgoyalevil could you please resolve conflicts? |
Signed-off-by: Ansh Goyal <[email protected]>
|
@yurishkuro Done 🚀 |
4 tasks
yurishkuro
pushed a commit
that referenced
this pull request
Aug 15, 2023
Related to: #1675 and #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 #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
added a commit
to anshgoyalevil/jaeger-ui
that referenced
this pull request
Aug 16, 2023
part of jaegertracing#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/testing-library__jest-dom` - `@types/react-copy-to-clipboard` - `@types/recompose` - `tar` ## 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
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]>
yurishkuro
added a commit
that referenced
this pull request
Aug 16, 2023
## Which problem is this PR solving? - Fixes #1672 ## Description of the changes - This PR adds a step in the unit-test workflow to check for unused dependencies using the depcheck package. - Certain dependencies which are installed but still being given in the output are put into `.depcheckrc.json` file to be ignored. - The `--skip-missing` flag is also added to ignore the missing deps warning ## How was this change tested? - The `ignores` array without any dep in `.depcheckrc.json` file was pushed to the branch for testing, and it was observed that the CI failed, which is the required behaviour: Workflow Run: https://github.com/anshgoyalevil/jaeger-ui/actions/runs/5854765474/job/15871258991 - The `ignores` array with all unused deps was pushed for CI testing, and the CI passed the Depcheck flow. Workflow Run: https://github.com/anshgoyalevil/jaeger-ui/actions/runs/5854816286/job/15871414318#step:6:10 (Ignore the Lint fail error, as it is resolved in this PR, but workflow run is not available because target push branch was changed to `main` in that commit, and thus workflow run is not available to be shared) ## Notes: Before merging this PR, all the unused deps which are neither added to the `ignores` array, not have been removed need to be removed. Thus PR #1675 needs to be merged before this one. After that, this PR's branch needs to be updated with that code so that the Depcheck doesn't fail in CI for this or for upcoming PRs. ## 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]> Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Pavol Loffay <[email protected]> Signed-off-by: wck-iipi <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Pavol Loffay <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Co-authored-by: Wck-iipi <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a part of #1672
What changes is this PR making?
@types/testing-library__jest-dom@types/react-copy-to-clipboard@types/recomposetarChecklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test