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

[Refactor Code] Remove unused services and imports #3547

Merged
merged 3 commits into from
Aug 12, 2021

Conversation

gautamjajoo
Copy link
Member

This PR removes unused imports increasing the bundle size and thus the time to load. Also removes the unused services.

@Ram81 @Kajol-Kumari

@codecov-commenter
Copy link

codecov-commenter commented Jul 25, 2021

Codecov Report

Merging #3547 (b0da28c) into master (96968d6) will decrease coverage by 0.76%.
The diff coverage is 34.17%.

@@            Coverage Diff             @@
##           master    #3547      +/-   ##
==========================================
- Coverage   72.93%   72.16%   -0.77%     
==========================================
  Files          83       20      -63     
  Lines        5368     3194    -2174     
==========================================
- Hits         3915     2305    -1610     
+ Misses       1453      889     -564     
Impacted Files Coverage Δ
frontend/src/js/controllers/authCtrl.js 53.91% <6.38%> (-12.95%) ⬇️
frontend/src/js/controllers/profileCtrl.js 79.76% <20.00%> (-13.10%) ⬇️
frontend/src/js/controllers/permissionCtrl.js 36.36% <22.22%> (-63.64%) ⬇️
frontend/src/js/controllers/challengeCtrl.js 64.68% <32.84%> (-9.02%) ⬇️
frontend/src/js/controllers/updateProfileCtrl.js 82.55% <44.44%> (-10.30%) ⬇️
frontend/src/js/controllers/challengeListCtrl.js 95.74% <50.00%> (+1.06%) ⬆️
...ntend/src/js/controllers/challengeHostTeamsCtrl.js 70.50% <66.66%> (-1.18%) ⬇️
frontend/src/js/controllers/teamsCtrl.js 71.17% <75.00%> (ø)
frontend/src/js/controllers/ChallengeInviteCtrl.js 100.00% <100.00%> (ø)
frontend/src/js/controllers/SubmissionFilesCtrl.js 95.45% <100.00%> (ø)
... and 30 more
Impacted Files Coverage Δ
frontend/src/js/controllers/authCtrl.js 53.91% <6.38%> (-12.95%) ⬇️
frontend/src/js/controllers/profileCtrl.js 79.76% <20.00%> (-13.10%) ⬇️
frontend/src/js/controllers/permissionCtrl.js 36.36% <22.22%> (-63.64%) ⬇️
frontend/src/js/controllers/challengeCtrl.js 64.68% <32.84%> (-9.02%) ⬇️
frontend/src/js/controllers/updateProfileCtrl.js 82.55% <44.44%> (-10.30%) ⬇️
frontend/src/js/controllers/challengeListCtrl.js 95.74% <50.00%> (+1.06%) ⬆️
...ntend/src/js/controllers/challengeHostTeamsCtrl.js 70.50% <66.66%> (-1.18%) ⬇️
frontend/src/js/controllers/teamsCtrl.js 71.17% <75.00%> (ø)
frontend/src/js/controllers/ChallengeInviteCtrl.js 100.00% <100.00%> (ø)
frontend/src/js/controllers/SubmissionFilesCtrl.js 95.45% <100.00%> (ø)
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9eb4de3...b0da28c. Read the comment docs.

Copy link
Member

@Kajol-Kumari Kajol-Kumari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @gautamjajoo as we are doing the code cleanup here so can we please fix the warning as well that we get while logging in?
steps to see the warning-

  1. Go to login Page
  2. Open up the console
  3. Enter credentials and hit login button

ref -
Screenshot 2021-07-25 at 9 22 19 PM

@gautamjajoo
Copy link
Member Author

@Kajol-Kumari I am opening up a separate PR for it. This issue is ony for dev mode and will not be there in production.
This is on the all-challenge-list page.

@gautamjajoo
Copy link
Member Author

@Kajol-Kumari I have fixed the above issue in this PR. This PR can be reviewed as well.

@Kajol-Kumari
Copy link
Member

Instead of manually finding and doing the cleanups everytime. Can we please add the "no-unused-variable": true and "noUnusedLocals": true in our tslint.json file of frontend_v2 ?

\cc: @Ram81 @RishabhJain2018

@gautamjajoo
Copy link
Member Author

@Kajol-Kumari I have used this in tsconfig.json and checked the errors,

"compilerOptions": {
  "noUnusedLocals": true,            /* Report errors on unused locals. */
  "noUnusedParameters": true     /* Report errors on unused parameters. */
}

I have added the above rules mentioned in tslint.json

@gautamjajoo gautamjajoo requested a review from Kajol-Kumari July 29, 2021 09:44
Copy link
Member

@Kajol-Kumari Kajol-Kumari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gautamjajoo let's not add the below configuration in tslint.json and instead add it in tsconfig.json file so that we can get the error during compilation only.

"compilerOptions": {
  "noUnusedLocals": true,            /* Report errors on unused locals. */
  "noUnusedParameters": true     /* Report errors on unused parameters. */
}

@gautamjajoo
Copy link
Member Author

@Kajol-Kumari There are some issues when we add these in tsconfig.json becuase we are using some services while it is showing error. Also tslint is deprecated now, we can try to move to eslint as well?

@Kajol-Kumari
Copy link
Member

@Kajol-Kumari There are some issues when we add these in tsconfig.json becuase we are using some services while it is showing error. Also tslint is deprecated now, we can try to move to eslint as well?

@gautamjajoo let's not do it rn. As per the scope of this PR adding it under tslint is also fine.

Copy link
Member

@Kajol-Kumari Kajol-Kumari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RishabhJain2018 RishabhJain2018 merged commit 797e1e7 into Cloud-CV:master Aug 12, 2021
vaheta pushed a commit to vahetag/EvalAI that referenced this pull request Dec 10, 2024
…d-CV#3547)

* remove unused services and imports

* add no-unused-variable in tslint

Co-authored-by: Rishabh Jain <[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 this pull request may close these issues.

5 participants