chore: Introduce Knip #18005

webpro · 2024-01-18T09:39:54Z

Prerequisites checklist

I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: add Knip, a project linter to keep codebase tidy

After having a "go" to explore the usefulness of Knip for the ESLint repository, and a meeting with @JoshuaKGoldberg the other day in which we've set up a first configuration, here's the follow up to that meeting in the form of a pull request.

EDIT: Knip is a project linter. Roughly speaking, where ESLint lints files in isolation, Knip lints codebases as a whole. This line could be drawn at the export keyword:

export const myExport = 1;

Knip creates a map of exports and imports to find unused exports. Additionally, it also finds unused dependencies, by mapping external imports against dependencies in package.json files. After all, it's only human to forget to delete unused files, exports and dependencies when they're no longer used. Knip is increasingly able to automate that process. Knip becomes smarter over time, resulting in less configuration. Using Knip in CI, just like ESLint, is beneficial to projects small and large to prevent regressions. Also see https://knip.dev/explanations/why-use-knip for some more in-depth explanations about what Knip has to offer. Knip has a lot more features than fit in this description, feel free to check them out on the website.

This PR is ready for review, but there's a few things I'd like you to decide upon:

Is Knip useful at all?
Do we want to merge the basics first (basically this PR as it stands with 2 commits)?
Do we want to actually clean up the codebase with the findings in this PR?
Do we want to add it the CI workflow in this PR?

What changes did you make? (Give an overview)

Add Knip + configuration (including comments)
Modified a few exports to accommodate for the ambiguity of CommonJS-styled (default) exports

This last bullet point might be seen as controversial. The thing is that Knip takes a rather conservative stance when it comes to named versus named exports in CommonJS modules. This is because the meaning of module.exports = is ambiguous (is it a default export or does it contain named exports?). The changes in this PR reflect that (I believe it's a small price to pay and validates Knip behavior).

Is there anything you'd like reviewers to focus on?

When running npm run lint:knip from the root, we'll see:

$ npm run lint:knip
Unused files (3)
lib/cli-engine/xml-escape.js
lib/shared/deprecation-warnings.js
tools/internal-testers/test-parser.js
Unused devDependencies (12)
dom-parser                     docs/package.json
eleventy-plugin-page-assets    docs/package.json
eleventy-plugin-reading-time   docs/package.json
imagemin                       docs/package.json
netlify-cli                    docs/package.json
eslint-plugin-eslint-comments  package.json     
eslint-plugin-jsdoc            package.json     
eslint-plugin-n                package.json     
eslint-plugin-unicorn          package.json     
lint-staged                    package.json     
webdriverio                    package.json     
yorkie                         package.json
Unlisted dependencies (1)
estree  lib/rules/no-useless-assignment.js
Unlisted binaries (1)
svgo  package.json
Unresolved imports (3)
../cli-engine/config-array  lib/linter/linter.js:61:35                         
./types                     lib/rules/utils/lazy-loading-rule-map.js:9:22      
../lib/shared/types         packages/js/src/configs/eslint-recommended.js:11:19
Unused exports (3)
default           unknown  lib/cli.js:492:18                 
isGlobPattern     unknown  lib/eslint/eslint-helpers.js:910:5
defineInMemoryFs  unknown  tests/_utils/index.js:61:5

Before actually cleaning up unused elements, I'd like to discuss a few results:

eslint-plugin-eslint-comments is installed and used in packages/eslint-config-eslint so I think can be removed from the root package.json
Is the mere presence of the "n/no-process-exit" rule enough to consider eslint-plugin-n a required plugin? Shouldn't n or eslint-plugin-n explicitly be added to the array of plugins?
How is lint-staged or the gitHooks in package.json actually used? lint-staged is reported since there's no reference to it (e.g. in one of the package.json#scripts

eslint-github-bot · 2024-01-18T09:40:00Z

Hi @webpro!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the pull request title to match this format, as we use it to generate changelogs and automate releases.

The commit message tag wasn't recognized. Did you mean "docs", "fix", or "feat"?
There should be a space following the initial tag and colon, for example 'feat: Message'.
The first letter of the tag should be in lowercase

To Fix: You can fix this problem by clicking 'Edit' next to the pull request title at the top of this page.

Read more about contributing to ESLint here

netlify · 2024-01-18T09:40:00Z

✅ Deploy Preview for docs-eslint ready!

Name	Link
🔨 Latest commit	`83de5e5`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/65ef22ea07d3d40008eb18ae
😎 Deploy Preview	https://deploy-preview-18005--docs-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

linux-foundation-easycla · 2024-01-18T09:40:03Z

The committers listed above are authorized under a signed CLA.

✅ login: webpro / name: Lars Kappert (055091c, 0889b43, 5baac95, 76dd0e8, 8a222a9, 9750f81, bb6de2b, 4c7bc82, 1d890c2, e655f22, f7d6b38, 7831590, e0781b0)
✅ login: JoshuaKGoldberg / name: Josh Goldberg ✨ (f04054a, 9aa8c63, b5881ee, 83de5e5, d6f4138, 23c4416)

nzakas

Thanks for putting this together. Overall, what this PR is missing is an explanation of what Knip is and what benefit it is to the project as a whole. If you can add that information into the original description, it would greatly facilitate conversation around this PR.

nzakas · 2024-01-18T20:21:51Z

knip.jsonc

@@ -0,0 +1,46 @@
+{


It would be helpful if you could explain what all of this does.

Most of the configuration comes down to providing entry files in order for Knip to do its thing.

This repository is much like a monorepo, but without explicitly defined workspaces as package managers like npm and pnpm recognize them. Knip normally hooks into package.json#workspaces, but those folders with a package.json manifest can also be manually added to the Knip configuration file. So the keys of the workspaces object are the paths to those folders.

nzakas · 2024-01-18T20:22:45Z

package.json

@@ -157,6 +160,7 @@
    "semver": "^7.5.3",
    "shelljs": "^0.8.2",
    "sinon": "^11.0.0",
+    "typescript": "^5.3.3",


Why do we need TypeScript?

Knip uses typescript and @types/node as peer dependencies to parse JavaScript and TypeScript files, build ASTs to find imports and exports, and as a foundation for module resolution. They're not regular dependencies of Knip as not to cause incompatibility issues with projects that do use TypeScript. Both devDependencies are already in node_modules, so they're not further impacting the size of the project.

webpro · 2024-01-18T21:58:42Z

Thanks for putting this together. Overall, what this PR is missing is an explanation of what Knip is and what benefit it is to the project as a whole. If you can add that information into the original description, it would greatly facilitate conversation around this PR.

Sorry, there was indeed no proper introduction. There's a new section added to the description.

Feel free to ask me anything you'd like to discuss or want to know more about.

JoshuaKGoldberg

Cool stuff! Left a few clarifying questions in lieu of a full review.

knip.jsonc

Makefile.js

knip.jsonc

package.json

webpro · 2024-01-23T07:17:56Z

To move this pull request forward, it would help to discuss questions number 2 and 3 I've listed at the end of the description. And then, eventually, the list of configuration options and things in the codebase we can delete.

mdjermanovic · 2024-01-23T07:59:54Z

eslint-plugin-eslint-comments is installed and used in packages/eslint-config-eslint so I think can be removed from the root package.json

Is the mere presence of the "n/no-process-exit" rule enough to consider eslint-plugin-n a required plugin? Shouldn't n or eslint-plugin-n explicitly be added to the array of plugins?

How is lint-staged or the gitHooks in package.json actually used? lint-staged is reported since there's no reference to it (e.g. in one of the package.json#scripts

1 and 2: These dependencies are needed when eslint is used with .eslintrc.js config file.

eslint/.eslintrc.js

Lines 64 to 66 in 8c1b8dd

    
           extends: [ 
        
               "eslint/eslintrc" 
        
           ],

eslint/packages/eslint-config-eslint/eslintrc.js

Lines 12 to 17 in 8c1b8dd

    
           extends: [ 
        
               "eslint:recommended", 
        
               "plugin:n/recommended", 
        
               "plugin:jsdoc/recommended", 
        
               "plugin:eslint-comments/recommended" 
        
           ],

We'll probably remove this config file and the dependencies soon (#18011).

yorkie creates git hooks and runs lint-staged.

webpro · 2024-02-01T17:41:04Z

1 and 2: These dependencies are needed when eslint is used with .eslintrc.js config file.

We'll probably remove this config file and the dependencies soon (#18011).

Let's await that then to circumvent unnecessary complexity/issues.

yorkie creates git hooks and runs lint-staged.

Added a yorkie plugin to Knip (and upgraded to that version here).

nzakas

LGTM. Interested to get this integrated and see what else we find along the way.

Would like @mdjermanovic and @JoshuaKGoldberg to approve before merging.

JoshuaKGoldberg

Progress! 🚀

I left a status summary comment on package.json. I think it'd make sense to perform the deletions as part of this PR so that the repo's in a clean slate. WDYT?

Requesting changes to also indicate the PR as blocked on #18011 per the comments.

JoshuaKGoldberg · 2024-02-02T17:32:46Z

tests/tools/config-rule.js

@@ -10,7 +10,7 @@
 //------------------------------------------------------------------------------

 const assert = require("chai").assert,
-    ConfigRule = require("../../tools/config-rule"),
+    { generateConfigsFromSchema } = require("../../tools/config-rule"),


The ConfigRule.createCoreRuleConfigs below also needs to be updated:

Suggested change

{ generateConfigsFromSchema } = require("../../tools/config-rule"),

{ createCoreRuleConfigs, generateConfigsFromSchema } = require("../../tools/config-rule"),

package.json

nzakas · 2024-02-06T17:48:10Z

It looks like the CI is breaking, can you take a look?

webpro · 2024-02-07T09:17:03Z

It looks like the CI is breaking, can you take a look?

I've fixed the issues to make tests pass. This is the only way I see that satisfies both proxyquire and knip.

knip.jsonc

nzakas · 2024-02-07T17:59:28Z

lib/shared/runtime-info.js

@@ -162,6 +162,7 @@ function version() {
 //------------------------------------------------------------------------------

 module.exports = {
+    __esModule: true,


This seems like a bad idea...what's the purpose of this?

First I refactored to this syntax, but proxquire does not play well with it (so tests were failing):

{ environment, version } = require("./shared/runtime-info"),

So I had no choice but to revert that change to use the "default import":

RuntimeInfo = require("./shared/runtime-info"),

That causes ambiguity in terms of intention, so I've added the property. I've explained more about this here:

chore: Introduce Knip #18005 (comment)

https://knip.dev/guides/commonjs

The name of the property could be anything, I think __esModule is common (in bundled code) and describes intent well, but if we think it might cause issues we can rename it.

Gotcha, makes sense. Can you add a comment above explaining that this property makes Knip treat the default export correctly? I could see people getting confused and perhaps unintentionally removing it in the future.

Good idea, done.

package.json

nzakas · 2024-02-13T18:13:28Z

@mdjermanovic still looking for your feedback here.

nzakas

LGTM. Would still like @mdjermanovic and @JoshuaKGoldberg to approve before merging.

JoshuaKGoldberg

This looks great to me!

I would love to get the list of unused reports down to 0 and add npm run lint:imports to CI. But if other folks are happy to have that be a followup, then I am too. This is a directly positive change! And, I've pestered @webpro quite a lot already for a very kind change. 😄

I asked in Discord about an issue template for repo tooling followups and can file followup issues for the existing comment threads.

🔥 🙌

JoshuaKGoldberg · 2024-02-16T18:08:02Z

knip.jsonc

+            ],
+            "ignoreDependencies": [
+                "c8",
+                // Ignore until Knip has a wdio plugin:


Suggested change

// Ignore until Knip has a wdio plugin:

// Ignore until Knip has a wdio plugin:

// https://github.com/webpro/knip/issues/483#issuecomment-1925751516

snitin315

Overall looks good. Can we:

Add a CI job for this to run on all commits?
Delete/remove unused files/exports/dependencies as reported in the output?

knip.jsonc

package.json

webpro · 2024-03-11T08:02:25Z

Just saying: since there are no more issues or blocking matters, I believe my work is done here. Please follow up with PRs for maintenance as you please.

fasttime · 2024-03-11T08:32:04Z

@webpro There are some suggestions on this issue that need to be addressed before it can be merged. Have you missed them or would you prefer not to put more work into this PR?

webpro · 2024-03-11T08:34:15Z

@fasttime The comments keep coming, where does it end? I prefer not to put more work in this PR.

fasttime · 2024-03-11T09:20:14Z

I prefer not to put more work in this PR.

Fine, thanks a lot for your time working on this! I really appreciate it.

At this point, we'll need someone else to finish the PR.

webpro · 2024-03-11T09:27:28Z

At this point, we'll need someone else to finish the PR.

Perhaps my badly worded point was that I think this PR is fine as it stands. The lights are green. The rest is more a matter of fine-tuning and other things not directly related to the intentions of this PR: introduce Knip. There will always be some maintenance in configuration, just like any other tool.

I wonder what must be done in order to start using Knip properly?

JoshuaKGoldberg · 2024-03-11T11:55:52Z

I can take over, if that's helpful. As webpro says - it looks like it's mostly fine-tuning at this point.

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

package.json

mdjermanovic

LGTM, thanks!

webpro changed the title ~~Introduce Knip~~ feat: Introduce Knip Jan 18, 2024

eslint-github-bot bot added the feature label Jan 18, 2024

webpro changed the title ~~feat: Introduce Knip~~ chore: Introduce Knip Jan 18, 2024

eslint-github-bot bot added the chore label Jan 18, 2024

webpro marked this pull request as ready for review January 18, 2024 10:05

webpro requested a review from a team as a code owner January 18, 2024 10:05

webpro mentioned this pull request Jan 18, 2024

Add Eleventy data directory as entry pattern webpro-nl/knip#454

Merged

nzakas reviewed Jan 18, 2024

View reviewed changes

JoshuaKGoldberg reviewed Jan 18, 2024

View reviewed changes

JoshuaKGoldberg mentioned this pull request Jan 19, 2024

Plugin for WebdriverIO ("wdio" / wdio.conf.js)? webpro-nl/knip#464

Closed

nzakas reviewed Jan 22, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

nzakas approved these changes Feb 1, 2024

View reviewed changes

JoshuaKGoldberg requested changes Feb 2, 2024

View reviewed changes

JoshuaKGoldberg reviewed Feb 2, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

nzakas reviewed Feb 7, 2024

View reviewed changes

nzakas approved these changes Feb 14, 2024

View reviewed changes

JoshuaKGoldberg approved these changes Feb 16, 2024

View reviewed changes

snitin315 reviewed Feb 16, 2024

View reviewed changes

knip.jsonc Show resolved Hide resolved

This was referenced Feb 16, 2024

Repo: Remove unused tools/update-rule-types.js #18121

Closed

Rule: require a meta.docs.recommended eslint-community/eslint-plugin-eslint-plugin#446

Closed

mdjermanovic reviewed Mar 6, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

voxpelli mentioned this pull request Mar 9, 2024

ci: add more linting checks eslint-community/eslint-utils#205

Merged

JoshuaKGoldberg and others added 3 commits March 11, 2024 10:43

Remove tools/update-rule-types.js from knip.jsonc

Loading
Loading status checks…

23c4416

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

Add corrected shared/types Rule import to lazy-loading-rule-map.js

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

d6f4138

Added back eslint-plugin-* devDependencies

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

f04054a

JoshuaKGoldberg reviewed Mar 11, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

JoshuaKGoldberg added 3 commits March 11, 2024 11:04

Rename to lint:unused

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

b5881ee

Merge branch 'main' into introduce-knip

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

9aa8c63

Fix introduced prism-eslint-hook complaints

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

83de5e5

mdjermanovic approved these changes Mar 11, 2024

View reviewed changes

mdjermanovic merged commit acc2e06 into eslint:main Mar 11, 2024
19 checks passed

webpro deleted the introduce-knip branch March 11, 2024 17:10

webpro mentioned this pull request Apr 5, 2024

chore: upgrade knip #18272

Merged

1 task

github-actions bot mentioned this pull request Apr 5, 2024

eslint/9.0.0 package update wolfi-dev/os#16409

Merged

data-sync-user mentioned this pull request Jun 3, 2024

Bump eslint from 8.57.0 to 9.0.0 MozillaFoundation/foundation.mozilla.org#12150

Closed

eslint-github-bot bot locked and limited conversation to collaborators Sep 8, 2024

eslint-github-bot bot added the archived due to age label Sep 8, 2024

	{ generateConfigsFromSchema } = require("../../tools/config-rule"),
	{ createCoreRuleConfigs, generateConfigsFromSchema } = require("../../tools/config-rule"),

	// Ignore until Knip has a wdio plugin:
	// Ignore until Knip has a wdio plugin:
	// https://github.com/webpro/knip/issues/483#issuecomment-1925751516

chore: Introduce Knip #18005

chore: Introduce Knip #18005

Conversation

webpro commented Jan 18, 2024 • edited Loading

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

What changes did you make? (Give an overview)

Is there anything you'd like reviewers to focus on?

eslint-github-bot bot commented Jan 18, 2024

netlify bot commented Jan 18, 2024 • edited Loading

✅ Deploy Preview for docs-eslint ready!

linux-foundation-easycla bot commented Jan 18, 2024 • edited Loading

nzakas left a 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

webpro commented Jan 18, 2024

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

webpro commented Jan 23, 2024

mdjermanovic commented Jan 23, 2024

webpro commented Feb 1, 2024

nzakas left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nzakas commented Feb 6, 2024

webpro commented Feb 7, 2024

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

nzakas commented Feb 13, 2024

nzakas left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snitin315 left a comment

Choose a reason for hiding this comment

webpro commented Mar 11, 2024

fasttime commented Mar 11, 2024

webpro commented Mar 11, 2024

fasttime commented Mar 11, 2024

webpro commented Mar 11, 2024

JoshuaKGoldberg commented Mar 11, 2024

mdjermanovic left a comment

Choose a reason for hiding this comment

webpro commented Jan 18, 2024 •

edited

Loading

netlify bot commented Jan 18, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jan 18, 2024 •

edited

Loading