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

[AC-1743] pt. 3 ⮕ Remove unused packages #399

Merged
merged 15 commits into from
Jan 16, 2024

Conversation

addisonbeck
Copy link
Contributor

@addisonbeck addisonbeck commented Jan 12, 2024

[AC-1743] pt. 3 ⮕ Remove unused packages

Type of change

  • Bug fix
  • New feature development
  • Tech debt (refactoring, code cleanup, dependency upgrades, etc)
  • Build/deploy pipeline (DevOps)
  • Other

🏁 Objective

As part of an initiative to audit dependencies across Directory Connector it makes sense to clean up a few unused packages from package.json.

🚩 The Problem

Directory Connector has a handful of completely unused packages. Some seem outdated, and some don't really make sense as to why there were ever here at all. A node dependency check tool called depcheck reports the following unused dependencies on main when running its default command depcheck:

Unused dependencies
* @angular/cli
* duo_web_sdk
Unused devDependencies
* @types/jest
* css-loader
* html-loader
* node-loader
* nodemon
* prebuild-install
* sass
* sass-loader
* tapable
* ts-loader
* ts-node
* tsconfig-paths
* ttypescript
* typemoq
* webpack-cli
* jest-junit
Missing dependencies
* google-auth-library: ./src/services/gsuite-directory.service.ts
* dotenv: ./scripts/notarize.js

Some of these are not exactly unused, but many are. This PR will run through this list and make accommodations as necessary for each package: either removing the package or asking depcheck to ignore it and rerunning the report.

🏃 The Solution

The following matrix outlines each listed package and the state it is in after these changes:

🙄 **Github formatted this weird.** Leaving it here for the commit message. See below comment for an easier to read version during PR review.
Package Name Is Used Fate Addressed by commit Notes
@angular/cli 🤷 🪓 9c1ec8e The angular cli is a dev cli that shouldn't ever be imported, so you could argue it should stay - but if you work on more than a single Angular app (which we do) it is redudant to have in each project and would be better installed globally if you want to use it. For this reason I removed it.
duo_web_sdk 🪓 cfef4f2 Directory Connector only supports API key login, which bypasses 2FA.
nodemon 🪓 f210cd9 nodemon is a hot-reload tool for node apps. It is unclear what this was ever used for in DC, but we don't need it.
prebuild-install 🪓 9205118 prebuild-install is a cli tool used in npm scripts. It is not used by any of Directory Connector's scripts.
tapable 🪓 baebf7c tappable is a utility package for writing webpack plugins. It is unused.
tsconfig-paths 🪓 907a06a We use a webpack plugin for this. This dependency is unused.
ttypescript 🪓 c9c400c ttypescript is a custom transformer tool that is unused by the project.
typemoq 🪓 acad908 typemoq is a testing tool that is unused by the project.
jest-junit 🪓 38d3c03 jest-junit is a jest reporter that at this time is not used by Directory Connecor. It has been removed.
ts-node 🪓 67d3217 ts-node is a typescript execution cli tool. It isn't used by any of Directory Connector's scripts or pipelines.
sass 👷 8c5dc90 Sass wasn't being detected becuase it wasn't explicitly being referenced in the webpack config for the loader that depends on it. sass-loader requires sass or another sass implementation to function, and recommends declaring which one in the config. I added an explicit declaration to our webpack config and this dependency was properly detected.
google-auth-library 👷 cf96ce5 google-auth-library was listed as a missing dependency. It's bundled with other dependencies we use, but we also explicitly reference it. I added an explicit reference to this package in package.json
dotenv 👷 14fc770 dotenv was listed as a missing dependency. We explicitly reference it during notorization, but depend on its presence via other packages. I added an explicit reference to this package in package.json
@types/jest 🙈 3e38641 This seems to be a bug with depcheck. Typechecking in tests fails without this package. I've configured it to be ignored.
css-loader 🙈 c57dc62 The way Directory Connector's webpack configs are named confuses depcheck's special parser for webpack plugins. I've configured it to ignore these.
html-loader 🙈 c57dc62 The way Directory Connector's webpack configs are named confuses depcheck's special parser for webpack plugins. I've configured it to ignore these.
node-loader 🙈 c57dc62 The way Directory Connector's webpack configs are named confuses depcheck's special parser for webpack plugins. I've configured it to ignore these.
sass-loader 🙈 c57dc62 The way Directory Connector's webpack configs are named confuses depcheck's special parser for webpack plugins. I've configured it to ignore these.
ts-loader 🙈 c57dc62 The way Directory Connector's webpack configs are named confuses depcheck's special parser for webpack plugins. I've configured it to ignore these.
webpack-cli 🙈 c57dc62 The webpack-cli is a CLI tool that our builds depend on, and it isn't detected properly by depcheclk. I've configured it to ignore this package.
After these changes depcheck reports no issues. Some notes:
  • Part of this work involved adding a .depcheckrc file to ignore some packages with the tool. I don't know if we'll use this consistently, so I was conflicted on adding a configuration file for it. I still think it's useful to have around, but will remove this if there are concerns.
  • Some packages were listed as "missing dependencies" and not "unused dependencies". This means depcheck found references to them in the source code, but no reference in package.json. We were importing these packages because they were already needed by other dependencies. In those cases I have added explicit references to package.json.
  • sass was explicitly added as our sass implementation for sass-loader.

⏭️ What's next?

Next up in this dependency upgrade effort I'll be:

  1. Performing any patch upgrades for outdated packages
  2. Performing any minor upgrades for outdated packages
  3. Performing any "safe" major upgrades for outdated packages
  4. Upgrading Angular, Webpack, Node, and npm to their latest LTS versions
  5. Handling the remaining major upgrades for outdated packages on a case-by-case basis
  6. Checking in with renovate to see what is still missing

Testing requirements

There are no specific testing requirements for this. QA will be doing a regression run of Directory Connector at the end of this effort.

Before you submit

  • I have checked for linting errors (npm run lint) (required)
  • I have added unit tests where it makes sense to do so (encouraged but not required)
  • This change requires a documentation update (notify the documentation team)
  • This change has particular deployment requirements (notify the DevOps team)

@addisonbeck
Copy link
Contributor Author

addisonbeck commented Jan 12, 2024

Package Name Is Used Fate Addressed by commit Notes
@angular/cli 🤷 🪓 9c1ec8e The angular cli is a dev cli that shouldn't ever be imported, so you could argue it should stay - but if you work on more than a single Angular app (which we do) it is redudant to have in each project and would be better installed globally if you want to use it. For this reason I removed it.
duo_web_sdk 🪓 cfef4f2 Directory Connector only supports API key login, which bypasses 2FA.
nodemon 🪓 f210cd9 nodemon is a hot-reload tool for node apps. It is unclear what this was ever used for in DC, but we don't need it.
prebuild-install 🪓 9205118 prebuild-install is a cli tool used in npm scripts. It is not used by any of Directory Connector's scripts.
tapable 🪓 baebf7c tappable is a utility package for writing webpack plugins. It is unused.
tsconfig-paths 🪓 907a06a We use a webpack plugin for this. This dependency is unused.
ttypescript 🪓 c9c400c ttypescript is a custom transformer tool that is unused by the project.
typemoq 🪓 acad908 typemoq is a testing tool that is unused by the project.
jest-junit 🪓 38d3c03 jest-junit is a jest reporter that at this time is not used by Directory Connecor. It has been removed.
ts-node 🪓 67d3217 ts-node is a typescript execution cli tool. It isn't used by any of Directory Connector's scripts or pipelines.
sass 👷 8c5dc90 Sass wasn't being detected becuase it wasn't explicitly being referenced in the webpack config for the loader that depends on it. sass-loader requires sass or another sass implementation to function, and recommends declaring which one in the config. I added an explicit declaration to our webpack config and this dependency was properly detected.
google-auth-library 👷 cf96ce5 google-auth-library was listed as a missing dependency. It's bundled with other dependencies we use, but we also explicitly reference it. I added an explicit reference to this package in package.json
dotenv 👷 14fc770 dotenv was listed as a missing dependency. We explicitly reference it during notorization, but depend on its presence via other packages. I added an explicit reference to this package in package.json
@types/jest 🙈 3e38641 This seems to be a bug with depcheck. Typechecking in tests fails without this package. I've configured it to be ignored.
css-loader 🙈 c57dc62 The way Directory Connector's webpack configs are named confuses depcheck's special parser for webpack plugins. I've configured it to ignore these.
html-loader 🙈 c57dc62 The way Directory Connector's webpack configs are named confuses depcheck's special parser for webpack plugins. I've configured it to ignore these.
node-loader 🙈 c57dc62 The way Directory Connector's webpack configs are named confuses depcheck's special parser for webpack plugins. I've configured it to ignore these.
sass-loader 🙈 c57dc62 The way Directory Connector's webpack configs are named confuses depcheck's special parser for webpack plugins. I've configured it to ignore these.
ts-loader 🙈 c57dc62 The way Directory Connector's webpack configs are named confuses depcheck's special parser for webpack plugins. I've configured it to ignore these.
webpack-cli 🙈 c57dc62 The webpack-cli is a CLI tool that our builds depend on, and it isn't detected properly by depcheclk. I've configured it to ignore this package.

@addisonbeck addisonbeck marked this pull request as ready for review January 12, 2024 20:34
@addisonbeck addisonbeck requested a review from a team as a code owner January 12, 2024 20:34
@addisonbeck addisonbeck requested review from vincentsalucci and eliykat and removed request for vincentsalucci January 12, 2024 20:34
eliykat
eliykat previously approved these changes Jan 15, 2024
.depcheckrc Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Seems fine to leave it here, it might be useful in the future. No harm if not.

@@ -151,7 +144,6 @@
"dependencies": {
"@angular/animations": "15.2.9",
"@angular/cdk": "15.2.9",
"@angular/cli": "15.2.9",
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, I don't think we've ever used the Angular CLI so I agree with removing this. (Imagine not handcrafting all your components! ha! but really there is some work required to align our repos with Angular CLI's expectations so it's not useful to us at the moment.)

Copy link
Member

Choose a reason for hiding this comment

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

Still lots here we can remove when we clean up old jslib code. But a good start 👏

Comment on lines +100 to +105
{
loader: "sass-loader",
options: {
implementation: require("sass"),
},
},
Copy link
Member

Choose a reason for hiding this comment

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

I think this should replace line 99 given that we're specifying the full "sass-loader" config object rather than just the shorthand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Resolved on 329be7a

@eliykat eliykat dismissed their stale review January 15, 2024 06:22

Meant to request changes to webpack.renderer.js

@addisonbeck addisonbeck requested a review from eliykat January 16, 2024 21:13
@addisonbeck addisonbeck merged commit 6e76f23 into main Jan 16, 2024
12 checks passed
@addisonbeck addisonbeck deleted the AC-1743/Remove-Unused-Dependencies branch January 16, 2024 22:01
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