Add and configure knip

[ericcornelissen]

Relates to #124
Reverts #163, #177

---

Summary

Add knip as a devDependency for vetting purposes and configure it with knip.json. knip is a vetting tool that "finds unused files, dependencies, and exports". Additionally, add a new CI job to run vetting tools.

[webpro]

With the latest alpha version of Knip (1.0.0-alpha.2) you could use this configuration:

{
  "entryFiles": ["lib/index.ts"],
  "projectFiles": ["lib/**/*.ts"],
  "ignoreBinaries": ["git", "shellcheck"],
  "eslint": {
    "sampleFiles": [
      "lib/index.ts",
      "package.json",
      ".markdownlint.yml",
      "README.md"
    ]
  }
}

This eslint config is rather specific to the custom configuration of this project and how ESLint works: used dependencies can only reliably be resolved through ESLint itself and this requires to feed it specific placeholder/sample files so ESLint will return corresponding config/plugins for that file matching a overrides pattern. By default Knip simply passed a placeholder.js and placeholder.ts which I think covers most cases.

There are still false positives, but the results are better imho. Most notably yaml-eslint-parser as an unlisted dependency, while it's a direct dependency of eslint-plugin-yml (because it's returned by ESLint as a parser which is usually a separate package).

Thanks for your patience. If anything, your input has helped Knip's development a lot.

[webpro]

One more thing... the entryFiles and projectFiles for "production mode" can be suffixed! like so:

{
  "entryFiles": ["lib/index.ts!"],
  "projectFiles": ["lib/**/*.ts!"],
  "ignoreBinaries": ["git", "shellcheck"],
  "eslint": {
    "sampleFiles": [
      "lib/index.ts",
      "package.json",
      ".markdownlint.yml",
      "README.md"
    ]
  }
}

This will give a single unused export (rules in lib/index.ts). Marking this as @public will result in a clean output for production when using knip --production:

/**
 * @public
 */
export const rules = {
  'no-top-level-variables': noTopLevelVariables,
  'no-top-level-side-effect': noTopLevelSideEffect
};

[ericcornelissen]

Thanks for having a look @webpro - I think I'll stick with the --production mode for now.

[webpro]

Something got me inspired to get rid of those sampleFiles, and then some. Thanks, this repo inspired me to add a few more features to Knip:

• Figure out the sampleFiles so no more need to configure these.
• Add plugins for mocha and stryker
• Add support for yaml/yml config files

Just saying, the yaml-eslint-parser is still listed as an unlisted dependency, as after reading the docs I think it should be installed and configured explicitly. I also understand things "just work" without doing this. For me this it's good to have an improved understanding of how far Knip could and should go (I could "fix" this particular instance, but that would also hide more issues). The remaining false positives (unused deps) are not in scope for Knip now.

{
  "entryFiles": ["lib/index.ts!"],
  "projectFiles": ["lib/**/*.ts!"],
  "ignoreBinaries": ["shellcheck"]
}

Given the above config and updating to the latest alpha. Before:

❯ npx knip
--- UNUSED DEV DEPENDENCIES (9)
@ericcornelissen/eslint-plugin-top   package.json
@stryker-mutator/mocha-runner        package.json
@stryker-mutator/typescript-checker  package.json
eslint-plugin-json                   package.json
eslint-plugin-markdown               package.json
eslint-plugin-yml                    package.json
eslint-v6                            package.json
eslint-v7                            package.json
ts-node                              package.json
--- UNLISTED DEPENDENCIES (1)
git  package.json

After:

❯ npx knip
--- UNUSED DEV DEPENDENCIES (3)
@ericcornelissen/eslint-plugin-top  package.json
eslint-v6                           package.json
eslint-v7                           package.json
--- UNLISTED DEPENDENCIES (1)
yaml-eslint-parser  .eslintrc.js

[ericcornelissen]

Nice progress @webpro - I tested out the latest release (1.0.0-alpha.3) and the outputs are as you said. However, it's incredibly slow all of the sudden, I'm getting these timings on my machine (see also the latest CI run):

npm run vet  21,37s user 51,07s system 184% cpu 39,163 total

Besides that, I agree with you that @ericcornelissen/eslint-plugin-top, eslint-v6, and eslint-v7 as unused dependencies is not unreasonable. I'm a bit confused by eslint-v8 not being reported as unused though.

[T]he yaml-eslint-parser is still listed as an unlisted dependency, as after reading the docs I think it should be installed and configured explicitly. I also understand things "just work" without doing this.

I can see where you're coming from, however yaml-eslint-parser a dependency of eslint-plugin-yml - not a peerDependency.

From my understanding, I think not having it as a dependency in this project is fine provided the parser is implicit in the plugin:yml/standard extension. If yaml-eslint-parser were specified explicitly, I'd agree it should be specified as a dependency here.

Having said that, I can see that 1) this could be hard to distinguish for knip and 2) it might make more sense for eslint-plugin-yml to make yaml-eslint-parser a peer dependency.

[webpro]

Nice progress @webpro - I tested out the latest release (1.0.0-alpha.3) and the outputs are as you said. However, it's incredibly slow all of the sudden, I'm getting these timings on my machine (see also the latest CI run):

npm run vet  21,37s user 51,07s system 184% cpu 39,163 total

That's odd, not experiencing that here. Running knip in either production mode or not with either version (alpha.2 or 3) takes around 3 seconds for me. The latest alpha.4 should have a faster production mode, though (went to 2 seconds here).

Besides that, I agree with you that @ericcornelissen/eslint-plugin-top, eslint-v6, and eslint-v7 as unused dependencies is not unreasonable. I'm a bit confused by eslint-v8 not being reported as unused though.

Nice catch. I found that eslint and eslint-vX all use the eslint binary. Knip just overwrote it every time it came across such a dependency, with eslint-v8 being the last of them, so that was still referenced and did not show up in the results. A proper fix isn't trivial, so for now I'm just considering all of them together. Assuming actual usage is then often quite custom just like in this repo. On the other hand, it loses the opportunity to point to unused/duplicate/obsolete dependencies.

[T]he yaml-eslint-parser is still listed as an unlisted dependency, as after reading the docs I think it should be installed and configured explicitly. I also understand things "just work" without doing this.

I can see where you're coming from, however yaml-eslint-parser a dependency of eslint-plugin-yml - not a peerDependency.

From my understanding, I think not having it as a dependency in this project is fine provided the parser is implicit in the plugin:yml/standard extension. If yaml-eslint-parser were specified explicitly, I'd agree it should be specified as a dependency here.

Having said that, I can see that 1) this could be hard to distinguish for knip and 2) it might make more sense for eslint-plugin-yml to make yaml-eslint-parser a peer dependency.

Agreed. If it was a peer dependency, Knip would be OK with it. This is a bit of an edge case though, and only applies to the eslint plugin.

[ericcornelissen]

That's odd, not experiencing that here. Running knip in either production mode or not with either version (alpha.2 or 3) takes around 3 seconds for me. The latest alpha.4 should have a faster production mode, though (went to 2 seconds here).

With the improvements following your previous comment I switched back to non-production analysis on this branch - so those numbers are for non-production use. I tested again with knip@1.0.0-alpha.6 and here is the --performance output from my machine:

Name                                size  min     max       median    sum     
----------------------------------  ----  ------  --------  --------  --------
findESLintDependencies                 2   60.71  37813.89  18937.30  37874.60
firstGlob                              7    3.43  36226.91      3.92  36252.66
findExternalImportModuleSpecifiers    13    0.06    861.86      0.66    876.94
resolveSourceFileDependencies          1  784.48    784.48    784.48    784.48
findReferences                         3    7.39    169.73      8.22    185.34
load                                   4    0.21     60.65      2.94     66.74
createProject                          2    6.62     37.62     22.12     44.24
glob                                   5    0.01     19.77      5.86     39.69
findManifestDependencies               1   13.48     13.48     13.48     13.48
pureGlob                               4    0.50      2.07      1.27      5.11
findStrykerDependencies                1    3.47      3.47      3.47      3.47
findMochaDependencies                  2    0.07      2.50      1.29      2.57
findDuplicateExportedNames            13    0.01      0.48      0.06      1.61
removeExternalSourceFiles              1    0.59      0.59      0.59      0.59
dirGlob                                1    0.31      0.31      0.31      0.31
findTypeScriptDependencies             1    0.25      0.25      0.25      0.25
getExportedDeclarations                7    0.00      0.04      0.02      0.11

Total running time: 40.4s

A proper fix isn't trivial, so for now I'm just considering all of them together. Assuming actual usage is then often quite custom just like in this repo. [...]

It's up to you/the implementation details of course but I'd prefer the eslint-vX dependencies as false positives in favor of potential false negatives (aka "it loses the opportunity to point to unused/duplicate/obsolete dependencies").

Agreed. If it was a peer dependency, Knip would be OK with it. This is a bit of an edge case though, and only applies to the eslint plugin.

I'm going to raise this upstream to see if that leads anywhere 🙂

[ericcornelissen]

@webpro, quick update from my side:

1. I upgraded to 1.0.0-beta.2 and the performance issues are gone.

2. Regarding yaml-eslint-parser:

   I'm going to raise this upstream to see if that leads anywhere

   didn't lead anywhere. I've just gone ahead and add it as a devDependency to this project which should work just fine I think.

3. I added $schema to my knip configuration and it works great except for the fact that my VSCode emits a warning that Property $schema is not allowed. I wasn't able to figure out what is going wrong. As a reference, I'm not encountering this problem with Stryker's schema. I opened an issue for this against the knip repository.

[codecov]

Codecov Report

Merging #127 (f571505) into main (d469274) will not change coverage.
The diff coverage is n/a.

❗ Current head f571505 differs from pull request most recent head ea42da9. Consider uploading reports for the commit ea42da9 to get more accurate results

@@            Coverage Diff            @@
##              main      #127   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            3         3           
  Lines          179       178    -1     
  Branches        35        35           
=========================================
- Hits           179       178    -1     

Impacted Files	Coverage Δ
lib/rules/no-top-level-side-effect.ts	100.00% <0.00%> (ø)

[webpro]

Thanks for the updates, Eric

It's up to you/the implementation details of course but I'd prefer the eslint-vX dependencies as false positives in favor of potential false negatives (aka "it loses the opportunity to point to unused/duplicate/obsolete dependencies").

I gave this another thought, and as you probably saw, went with what you prefer :)