fix!: check for missing production dependencies (#1426)

Runs the dep-check command twice - once omitting dev deps and only checking non-test code, then again but only testing test code. [depcheck](https://www.npmjs.com/package/depcheck) is a bit wonky in that it doesn't support whitelisting files to check, only blacklisting so we pass a configurable list of ignore patterns that exclude known test files while checking prod deps and then a list of known prod files while checking dev deps. Should prevent future bugs similar to ipfs/helia-ipns#159 BREAKING CHANGE: previously undetected missing or unused deps will cause the dep-check command to fail
ipfs · Jan 6, 2024 · a66384a · a66384a
1 parent f8e63d3
commit a66384a
Show file tree

Hide file tree

Showing 6 changed files with 116 additions and 38 deletions.
diff --git a/package.json b/package.json
@@ -305,14 +305,12 @@
     "strip-bom": "^5.0.0",
     "strip-json-comments": "^5.0.0",
     "strong-log-transformer": "^2.1.0",
-    "tempy": "^3.0.0",
+    "tempy": "^3.1.0",
     "typedoc": "^0.25.0",
     "typedoc-plugin-mdn-links": "^3.0.3",
     "typedoc-plugin-missing-exports": "^2.0.0",
     "typescript": "^5.1.6",
     "typescript-docs-verifier": "^2.5.0",
-    "uint8arrays": "^5.0.1",
-    "undici": "^6.2.1",
     "update-notifier": "^7.0.0",
     "wherearewe": "^2.0.1",
     "yargs": "^17.1.1",
@@ -335,7 +333,9 @@
     "@types/strong-log-transformer": "^1.0.2",
     "@types/update-notifier": "^6.0.1",
     "@types/yargs": "^17.0.0",
-    "electron": "^27.0.2"
+    "electron": "^27.0.2",
+    "uint8arrays": "^5.0.1",
+    "undici": "^6.2.1"
   },
   "browser": {
     "fs": false,

diff --git a/src/cmds/dependency-check.js b/src/cmds/dependency-check.js
@@ -39,6 +39,18 @@ export default {
         default: true,
         type: 'boolean'
       })
+      .option('P', {
+        alias: 'productionIgnorePatterns',
+        describe: 'Patterns to ignore while checking production dependencies',
+        array: true,
+        default: userConfig.dependencyCheck.productionIgnorePatterns
+      })
+      .option('D', {
+        alias: 'developmentIgnorePatterns',
+        describe: 'Patterns to ignore while checking development dependencies',
+        array: true,
+        default: userConfig.dependencyCheck.developmentIgnorePatterns
+      })
   },
   /**
    * @param {any} argv

diff --git a/src/config/user.js b/src/config/user.js
@@ -114,7 +114,18 @@ const defaults = {
   // dependency check cmd options
   dependencyCheck: {
     unused: false,
-    ignore: []
+    ignore: [],
+    productionIgnorePatterns: [
+      '/benchmark',
+      '/benchmarks',
+      '/dist',
+      '/test',
+      '.aegir.js'
+    ],
+    developmentIgnorePatterns: [
+      '/dist',
+      '/src'
+    ]
   },
   exec: {
     bail: true,

diff --git a/src/dependency-check.js b/src/dependency-check.js
@@ -1,6 +1,8 @@
 /* eslint-disable no-console */
 
-import { cwd } from 'process'
+import fs from 'node:fs'
+import path from 'node:path'
+import { cwd } from 'node:process'
 import depcheck from 'depcheck'
 import kleur from 'kleur'
 import Listr from 'listr'
@@ -31,51 +33,92 @@ const tasks = new Listr(
        * @param {Task} task
        */
       task: async (ctx, task) => {
-        const result = await depcheck(cwd(), {
+        let missingOrUnusedPresent = false
+
+        // check production dependencies
+        const manifest = JSON.parse(fs.readFileSync(path.join(cwd(), 'package.json'), 'utf-8'))
+
+        const productionOnlyResult = await depcheck(cwd(), {
           parsers: {
             '**/*.js': depcheck.parser.es6,
             '**/*.ts': depcheck.parser.typescript,
             '**/*.cjs': depcheck.parser.es6,
             '**/*.mjs': depcheck.parser.es6
           },
-          ignoreMatches: ignoredDevDependencies.concat(ctx.fileConfig.dependencyCheck.ignore).concat(ctx.ignore)
+          ignoreMatches: ignoredDevDependencies.concat(ctx.fileConfig.dependencyCheck.ignore).concat(ctx.ignore),
+          ignorePatterns: ctx.productionIgnorePatterns,
+          package: {
+            ...manifest,
+            devDependencies: {}
+          }
         })
 
-        if (Object.keys(result.missing).length > 0 || result.dependencies.length > 0 || result.devDependencies.length > 0) {
-          if (Object.keys(result.missing).length > 0) {
-            console.error('')
-            console.error('Missing dependencies:')
-            console.error('')
+        if (Object.keys(productionOnlyResult.missing).length > 0) {
+          missingOrUnusedPresent = true
+          console.error('')
+          console.error('Missing production dependencies:')
+          console.error('')
 
-            Object.entries(result.missing).forEach(([dep, path]) => {
-              console.error(kleur.red(dep))
-              console.error(' ', kleur.gray(path.join('\n  ')))
-            })
-          }
+          Object.entries(productionOnlyResult.missing).forEach(([dep, path]) => {
+            console.error(kleur.red(dep))
+            console.error(' ', kleur.gray(path.join('\n  ')))
+          })
 
-          if (result.dependencies.length > 0) {
-            console.error('')
-            console.error('Unused production dependencies:')
-            console.error('')
+          console.error('')
+        }
 
-            result.dependencies.forEach(dep => {
-              console.error(kleur.yellow(dep))
-            })
-          }
+        if (productionOnlyResult.dependencies.length > 0) {
+          missingOrUnusedPresent = true
+          console.error('')
+          console.error('Unused production dependencies:')
+          console.error('')
 
-          if (result.devDependencies.length > 0) {
-            console.error('')
-            console.error('Unused dev dependencies:')
-            console.error('')
+          productionOnlyResult.dependencies.forEach(dep => {
+            console.error(kleur.yellow(dep))
+          })
 
-            result.devDependencies.forEach(dep => {
-              console.error(kleur.yellow(dep))
-            })
-          }
+          console.error('')
+        }
+
+        // check dev dependencies
+        const devlopmentOnlyResult = await depcheck(cwd(), {
+          parsers: {
+            '**/*.js': depcheck.parser.es6,
+            '**/*.ts': depcheck.parser.typescript,
+            '**/*.cjs': depcheck.parser.es6,
+            '**/*.mjs': depcheck.parser.es6
+          },
+          ignoreMatches: ignoredDevDependencies.concat(ctx.fileConfig.dependencyCheck.ignore).concat(ctx.ignore),
+          ignorePatterns: ctx.developmentIgnorePatterns
+        })
+
+        if (Object.keys(devlopmentOnlyResult.missing).length > 0) {
+          missingOrUnusedPresent = true
+          console.error('')
+          console.error('Missing development dependencies:')
+          console.error('')
 
-          // necessary because otherwise listr removes the last line of output
-          console.error(' ')
+          Object.entries(devlopmentOnlyResult.missing).forEach(([dep, path]) => {
+            console.error(kleur.red(dep))
+            console.error(' ', kleur.gray(path.join('\n  ')))
+          })
+
+          console.error('')
+        }
+
+        if (devlopmentOnlyResult.devDependencies.length > 0) {
+          missingOrUnusedPresent = true
+          console.error('')
+          console.error('Unused development dependencies:')
+          console.error('')
+
+          devlopmentOnlyResult.devDependencies.forEach(dep => {
+            console.error(kleur.yellow(dep))
+          })
+          console.error('')
+        }
 
+        if (missingOrUnusedPresent) {
           throw new Error('Some dependencies are missing or unused')
         }
       }

diff --git a/src/types.ts b/src/types.ts
@@ -350,14 +350,26 @@ interface ReleaseRcOptions {
 
 interface DependencyCheckOptions {
   /**
-   * throws error on unused dependencies, default is false
+   * throws error on unused dependencies
+   *
+   * @default true
    */
   unused: boolean
+
   /**
    * Ignore these dependencies when considering which are used and which are not
    */
   ignore: string[]
 
+  /**
+   * Files to ignore when checking production dependencies
+   */
+  productionIgnorePatterns: string[]
+
+  /**
+   * Files to ignore when checking dev dependencies
+   */
+  developmentIgnorePatterns: string[]
 }
 
 interface ExecOptions {

diff --git a/test/dependency-check.js b/test/dependency-check.js
@@ -76,7 +76,7 @@ describe('dependency check', () => {
   /**
    * depcheck removed this option as it caused too many false positives
    */
-  it.skip('should fail for missing production deps', async () => {
+  it('should fail for missing production deps', async () => {
     await expect(
       execa(bin, ['dependency-check', '-p'], {
         cwd: path.join(__dirname, 'fixtures/dependency-check/fail-prod')