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

[Feature]: Add CI step to check dependencies #1672

Closed
yurishkuro opened this issue Aug 13, 2023 · 6 comments · Fixed by #1677
Closed

[Feature]: Add CI step to check dependencies #1672

yurishkuro opened this issue Aug 13, 2023 · 6 comments · Fixed by #1677

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Aug 13, 2023

Recently we discovered several dependencies that were listed in the package.json but were not actually used (e.g. #1669, #1616, #1637). It would be good to have a CI-time check that would fail the CI when unused or missing dependencies are discovered.

I don't know if depcheck module is the best solution for this, but when I ran it in packages/jaeger-ui, it printed:

# ...jaeger-ui/packages/jaeger-ui 
$ depcheck
Unused dependencies
* @types/react-copy-to-clipboard
* @types/recompose
* d3-scale
* redux-async-middleware
* tar
Unused devDependencies
* @babel/preset-env
* @babel/preset-react
* @babel/preset-typescript
* @svgr/babel-plugin-transform-svg-component
* @svgr/babel-preset
* @types/react-dom
* @types/react-router-redux
* babel-plugin-inline-react-svg
* jest-environment-jsdom
* react-test-renderer
* source-map-explorer
Missing dependencies
* react-select: ./src/components/common/VirtSelect.tsx
* react-virtualized: ./src/components/common/VirtSelect.tsx

I don't think it's completely accurate, especially some of devDependencies may be used in the build configurations and are a false positive. Maybe it can be additionally tuned/configured, or perhaps we can add a dummy "tools.js" file that will import these modules so that they will not be flagged.

The new step should:

  • run the check in both jaeger-ui and plexus workspaces
  • fail if missing or unused dependencies are found
  • before failing, print a recommendation on how to add or remove them, e.g. cd packages/jaeger-ui; yarn remove $pkg
  • maybe it doesn't need to be a separate GH Action, but run as part of yarn lint
@yurishkuro
Copy link
Member Author

@shubbham1215 I mean automatically invoke it when running yarn lint, then it will already be a part of CI

@yurishkuro yurishkuro transferred this issue from jaegertracing/jaeger Aug 13, 2023
@anshgoyalevil
Copy link
Member

@yurishkuro I have worked with this previously PalisadoesFoundation/talawa-admin#858 (comment) and the problem with that is it sometimes gives false positives.

@yurishkuro
Copy link
Member Author

I think there are ways to deal with false positives. For example, if a package is only referenced in the linter configuration, will removing it fail the linter?

I can see writing a script that runs before depcheck, scans the other config files that depcheck does not understand, and generates a temporary exclusion file based on our understanding of those config files.

@anshgoyalevil
Copy link
Member

As per my analysis:

Unused dependencies
* @types/react-copy-to-clipboard // Can be removed safely
* @types/recompose // Can be removed safely
* tar // False Positive (Being used inside the release.yml for packaging into tar)
Unused devDependencies
* @babel/preset-env // (false +ve)
* @babel/preset-react // (false +ve)
* @babel/preset-typescript // (false +ve. These babel plugins are being used inside babel-transform.js)
* @svgr/babel-plugin-transform-svg-component // can be removed
* @svgr/babel-preset // can be removed
* @types/react-dom // should not be removed, but unsure
* @types/react-router-redux //shouldn't be removed since we are using react-router-redux
* @types/testing-library__jest-dom //direct dep can be removed, as it is already a dep for jest-dom
* babel-plugin-inline-react-svg // false +ve
* jest-environment-jsdom // false +ve
Missing dependencies // can be ignored using --skip-missing
* react-is: ./src/utils/ReactShallowRenderer.test.js
* react-select: ./src/components/common/VirtSelect.tsx
* react-virtualized: ./src/components/common/VirtSelect.tsx
* _: ./build/static/index-21cea8b1.js

@yurishkuro
Copy link
Member Author

tar // False Positive (Being used inside the release.yml for packaging into tar)

in release.yml we use tar the Unix utility, not tar the npm module.

@anshgoyalevil
Copy link
Member

anshgoyalevil commented Aug 14, 2023

In that case, it can also be removed. Creating a PR to remove unused packages. Further, would create a script for detecting false positives.

yurishkuro pushed a commit that referenced this issue Aug 14, 2023
<!--
!! Please DELETE this comment before posting.
We appreciate your contribution to the Jaeger project! 👋🎉
-->
Fixes a part of #1672 

## What changes is this PR making?
- This PR removes the unused `redux-async-middleware` package.

## 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]>
Wck-iipi pushed a commit to Wck-iipi/jaeger-ui that referenced this issue Aug 14, 2023
<!--
!! Please DELETE this comment before posting.
We appreciate your contribution to the Jaeger project! 👋🎉
-->
Fixes a part of jaegertracing#1672

## What changes is this PR making?
- This PR removes the unused `redux-async-middleware` package.

## 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: wck-iipi <[email protected]>
yurishkuro pushed a commit that referenced this issue Aug 15, 2023
part of #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]>
yurishkuro pushed a commit that referenced this issue 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 issue 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 issue 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 issue 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants