Unused dependency checker and fixer #23989

pragya91 · 2025-03-05T14:30:41Z

Purpose: Report and fix unused dependencies.

This PR adds a new check scripts check:knip to check and knip:fix to fix, the unused dependencies found in the package.json of any the packages in the repo . The underlying tool used is knip npm package.
For now, we only introduce this script to one of the packages - build-cli. The idea is to incrementally make the same change to all the packages in our repo, to ensure quicker PR turnaround time and to open up possibility of multiplying the effort with more devs.

Future of this work

Add knip and fix scripts to all the packages progressively.
Possibly add a policy check handler to check if the script has been added to all the packages, just like we have for 'prettier'.
Once we have completed a basic for knip - dependency checker tool, and it is found to be a stable one to use with our repo, we can expand the implementation to make use of more features knip provides - like checking for unused files, unused exports etc.

Context
(This work is continuation of the previous hack project #22448 and also the work done by Tyler in the PR here: #23992 where we explore the various build/reporting options that work the best with our requirements.)

jason-ha

Can you generate a PR from cleanup in client? Will be very helpful to see what we are getting with this policy.

build-tools/packages/build-cli/src/library/repoPolicyCheck/npmPackages.ts

fluidBuild.config.cjs

tylerbutler

Thanks for putting this together! In #22448 (comment) I outlined three options for this type of check. This PR is option 2, and I got curious about option 1, which is doing a per-package integration at the task level - adding new depcheck tasks to individual packages and then running them as part of the build.

I looked into depcheck's config story, and there is no inheritance or looking up the tree for configs. It's pretty clearly a "one package at a time" tool. However, we can create our own base config and import it and modify it on a per-package basis since the config supports CJS. So the next question is, "can we share a config that the majority of packages would use?"

The answer to that is a little unclear, but based on my testing, it doesn't look good unfortunately. The problem is that there are a lot of false results. Depcheck flags some deps that are used at runtime or in CI, etc. I suspect each package in the repo will need some level of config. That means a config file in each package, even if it mostly inherits from the root config. I suspect many packages will have additional exclusions that apply only to it as well.

These false positives also make it unlikely that an autofixer will be safe. Each removal needs to be scrutinized.

All of this together points towards option 1 possibly being a better approach for this integration. I explored those changes in #23992. Note that many of those changes would be needed with this approach as well, since each package will need its own config.

build-tools/packages/build-cli/src/commands/check/policy.ts

build-tools/packages/build-cli/src/library/repoPolicyCheck/npmPackages.ts

pragya91 · 2025-03-07T21:36:31Z

@tylerbutler / @jason-ha - Thanks for reviewing the first pass of the implementation.
The PR has a new look now, where we are changing our direction from adding a new policy check handler to having check scripts instead. The underlying tool is also changed from depcheck to knip since it is more actively maintained and is more feature rich. ( Thanks @sonalivdeshpande as the original pitcher for this idea and @tylerbutler for reviewing).

Requesting a re-review. Please let me know if we can make further improvements to our approach. Thanks!

pragya91 · 2025-03-07T21:39:01Z

build-tools/packages/build-cli/package.json

@@ -43,6 +43,8 @@
 		"build:test:esm": "tsc --project ./src/test/tsconfig.json",
 		"check:biome": "biome check .",
 		"check:format": "npm run check:biome",
+		"check:knip": "knip --no-config-hints",
+		"knip:fix": "knip --fix --fix-type dependencies --no-config-hints",


--no-config-hints -- supresses the hints provided by knip tool to remove the unused ignoreDependencies coming from knip.base.config.

It's mildly annoying the fix-types aren't part of config. DOesn't seem to be supported.

tylerbutler · 2025-03-07T22:50:38Z

build-tools/packages/build-cli/knip.config.ts

+// Import the shared config from the root of the repo.
+import sharedConfig from "../../../knip.base.ts";
+
+export default {


Add a doc comment with a type directive here so we can get type checking on the config object. Something like this:

/** * @type {import("knip").Config} */

Oh right it's typescript - just import type then.

And you can remove the ts-check-related comments.

tylerbutler · 2025-03-07T22:58:25Z

build-tools/packages/build-cli/knip.config.ts

+import sharedConfig from "../../../knip.base.ts";
+
+export default {
+	include: [...sharedConfig.include],


Suggested change

include: [...sharedConfig.include],

...sharedConfig,

tylerbutler · 2025-03-07T23:10:56Z

build-tools/packages/build-cli/package.json

-		"@andrewbranch/untar.js": "^1.0.3",
-		"@fluid-tools/build-infrastructure": "workspace:~",
-		"@fluid-tools/version-tools": "workspace:~",


this is definitely a false positive. Does it understand the workspace:~ deps?

tylerbutler · 2025-03-07T23:11:17Z

build-tools/packages/build-cli/package.json

-		"@fluidframework/bundle-size-tools": "workspace:~",
-		"@inquirer/prompts": "^7.0.1",


Likely a false positive.

tylerbutler · 2025-03-07T23:14:05Z

build-tools/packages/build-cli/package.json

I think most of the removed deps are actually used.

github-actions bot added area: build Build related issues base: main PRs targeted against main branch labels Mar 5, 2025

pragya91 changed the title ~~check deps~~ Used dependency checker and fixer Mar 5, 2025

pragya91 requested review from tylerbutler, alexvy86, jason-ha and sonalideshpandemsft March 5, 2025 18:41

pragya91 changed the title ~~Used dependency checker and fixer~~ Unused dependency checker and fixer Mar 5, 2025

pragya91 requested review from a team, markfields, jatgarg, kian-thompson, WillieHabi, rajatch-ff and MarioJGMsoft and removed request for a team March 5, 2025 18:45

pragya91 marked this pull request as ready for review March 5, 2025 19:54

Copilot bot review requested due to automatic review settings March 5, 2025 19:54

This comment was marked as spam.

Sign in to view

jason-ha reviewed Mar 5, 2025

View reviewed changes

build-tools/packages/build-cli/src/library/repoPolicyCheck/npmPackages.ts Outdated Show resolved Hide resolved

build-tools/packages/build-cli/src/library/repoPolicyCheck/npmPackages.ts Outdated Show resolved Hide resolved

fluidBuild.config.cjs Outdated Show resolved Hide resolved

tylerbutler mentioned this pull request Mar 5, 2025

build: Add depcheck tasks to some packages #23992

Closed

tylerbutler reviewed Mar 5, 2025

View reviewed changes

build-tools/packages/build-cli/src/commands/check/policy.ts Outdated Show resolved Hide resolved

build-tools/packages/build-cli/src/library/repoPolicyCheck/npmPackages.ts Outdated Show resolved Hide resolved

pragya91 closed this Mar 5, 2025

pragya91 reopened this Mar 5, 2025

github-actions bot added the dependencies Pull requests that update a dependency file label Mar 7, 2025

pragya91 added 5 commits March 7, 2025 13:23

init

7771f8d

Working reports and --fix command

537fb36

reset package.json

b5abd14

undefined check

8204812

Knip for dep checking - working example for build-cli package

532f07a

pragya91 force-pushed the praggarg/deps branch from 6b4c808 to 532f07a Compare March 7, 2025 21:30

pragya91 commented Mar 7, 2025

View reviewed changes

tylerbutler reviewed Mar 7, 2025

View reviewed changes

build-tools/packages/build-cli/package.json

Copy link

Member

tylerbutler Mar 7, 2025

Choose a reason for hiding this comment

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

I think most of the removed deps are actually used.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unused dependency checker and fixer #23989

Unused dependency checker and fixer #23989

pragya91 commented Mar 5, 2025 •

edited

Loading

This comment was marked as spam.

jason-ha left a comment

tylerbutler left a comment

pragya91 commented Mar 7, 2025

pragya91 Mar 7, 2025

tylerbutler Mar 7, 2025

tylerbutler Mar 7, 2025

tylerbutler Mar 7, 2025

tylerbutler Mar 7, 2025

tylerbutler Mar 7, 2025

tylerbutler Mar 7, 2025

tylerbutler Mar 7, 2025

tylerbutler Mar 7, 2025

		"@fluidframework/bundle-size-tools": "workspace:~",
		"@inquirer/prompts": "^7.0.1",

Unused dependency checker and fixer #23989

Are you sure you want to change the base?

Unused dependency checker and fixer #23989

Conversation

pragya91 commented Mar 5, 2025 • edited Loading

This comment was marked as spam.

jason-ha left a comment

Choose a reason for hiding this comment

tylerbutler left a comment

Choose a reason for hiding this comment

pragya91 commented Mar 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pragya91 commented Mar 5, 2025 •

edited

Loading