Add policy to detect unused dependencies
The goal of this PR is to introduce a policy that detects and reports unused dependencies in packages. It introduces a new policy called npm-check-unused-dependencies, leveraging the depcheck tool to identify unused dependencies across packages.
For now, the policy is excluded from fluidBuild.config.cjs as temporarily, enabling a phased approach where the policy is applied incrementally, one package at a time. This prevents failures in flub policy-check whenever a new release of build-tools is consumed.
The policy will be fully enabled once all the unused dependencies are removed or added to a .depcheckrc file.
⯅ @fluid-example/bundle-size-tests: +245 Bytes
| Metric Name | Baseline Size | Compare Size | Size Diff |
|---|---|---|---|
| aqueduct.js | 460.44 KB | 460.47 KB | ⯅ +35 Bytes |
| azureClient.js | 559.03 KB | 559.08 KB | ⯅ +49 Bytes |
| connectionState.js | 680 Bytes | 680 Bytes | ■ No change |
| containerRuntime.js | 261.68 KB | 261.69 KB | ⯅ +14 Bytes |
| fluidFramework.js | 403.45 KB | 403.47 KB | ⯅ +14 Bytes |
| loader.js | 134.17 KB | 134.19 KB | ⯅ +14 Bytes |
| map.js | 42.43 KB | 42.44 KB | ⯅ +7 Bytes |
| matrix.js | 145.87 KB | 145.88 KB | ⯅ +7 Bytes |
| odspClient.js | 526.18 KB | 526.23 KB | ⯅ +49 Bytes |
| odspDriver.js | 97.8 KB | 97.82 KB | ⯅ +21 Bytes |
| odspPrefetchSnapshot.js | 42.76 KB | 42.78 KB | ⯅ +14 Bytes |
| sharedString.js | 162.84 KB | 162.84 KB | ⯅ +7 Bytes |
| sharedTree.js | 393.92 KB | 393.92 KB | ⯅ +7 Bytes |
| Total Size | 3.3 MB | 3.3 MB | ⯅ +245 Bytes |
Baseline commit: 2f7a9d0861c16dfc51042509c4dffa6dff2b6e33
Generated by :no_entry_sign: dangerJS against d069bdeb7b92b06a964aa69b2b5b18ee010e7b9e
- I see pros and cons to this, but did we consider having an npm script on each package that runs depcheck, making it part of the build, and making the policy just a check for "packages must have this script"?
Since both Alejandro and Jason have brought this up, I think it's worth expanding on the three high-level options for adding this sort of functionality to the build:
- Add scripts to each package that invoke the depcheck CLI directly. The scripts are typically accompanied by project-specific config files that ideally inherit from a shared config file stored centrally in the repo. The new scripts are then configured to run as part of the build by updating the fluid-build task config. Examples of these sorts of tasks are the prettier/biome formatting tasks and all the scripts that call api-extractor. An advantage to this approach is that it's completely config based. There's no "code," per se, since we invoke the tool using the CLI it provides. However, this approach relies on the configurability of the tool in question. If the CLI and config options it provides aren't sufficient for our use-case, we may have better luck with one of the other options.
- Wrap the tool in a policy, which is the option taken in this PR. This option is the least amount of "churn" in the repo overall, because it requires minimal config changes - once the new policy is rolled out, the only config changes needed are to opt individual packages in/out of the policy. Policy provides ways to opt in/out of the policy at the file and per-policy levels. When a new package is added, the policy will immediately apply to it - no one needs to remember to add a particular script. This advantage points to its disadvantage, though - policies apply to the whole repo, so when we intend a policy only for the client release group, we have to exclude the server and other files from that policy. That said, it is easy to document these exceptions, and they can apply only to the newly added policy. Examples of this type of approach is our sort-package-json policy. We could have used the sort-package-json CLI, but doing it as a policy has proven to require very little maintenance.
- Write a flub command that wraps the tool. This is similar to option 2 but takes it to the extreme - essentially we (re)implement a CLI wrapper around the tool's API. If the tool already provides a CLI, this is typically unnecessary work. But there are cases where there is no CLI, or where we want to run the tool in different places and it's easier to configure it using our own mechanisms.
The question raised above are asking about option 1 and why we prefer option 2. It's a reasonable question. To answer it, I think we need to examine:
- Can we avoid loading config files if we use a policy? For example, could the config be centralized and in our code we always load it from one place? If not - if all packages need their own config file - then the advantages of using policy drop.
- Does each package need separate config? If so, then using the depcheck CLI might be clearer to folks in the future since we'd be using depcheck "as documented."
- As Jason asked, can we provide an auto-fixer (I may not have covered these when we talked about policy a few weeks ago) for the policy? If so, then that's a nice advantage of the policy approach. Even if the fixer can't fix everything, fixing some stuff would be helpful. An example might be auto-removing the mocha dep because we know the incidence of false-positives with that particular depcheck is low. Just an example.
Regardless of everything else, I also want to echo some of the other feedback:
-
When reviewing a new policy, it's very helpful to see the effects of that policy on the client release group. You would ideally also pre-apply any changes from the policy before rolling it out. To run the policy locally to see its effects, you can link the client workspace to the build-tools and build-cli packages. This command will make the updates to the root package.json:
npm pkg set pnpm.overrides.@fluidframework/build-tools=link:./build-tools/packages/build-tools pnpm.overrides.@fluid-tools/build-cli=link:./build-tools/packages/build-cliThen run
pnpm i --no-frozen-lockfileto link the deps. Once done, if you run policy-check:fix in the root, it will invoke the local version (make sure build-tools is built before running any build commands in the root). You can then commit those changes into a separate PR and link to it from this one to demonstrate what the changes will look like.
This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!