Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: replace ts-prune with knip #174

Merged
merged 16 commits into from Jan 20, 2023

Conversation

webpro
Copy link
Contributor

@webpro webpro commented Jan 8, 2023

Noticed the other day (from release-it/release-it#964) that this repo might benefit from Knip, so here I am introducing Knip to find unused files, dependencies and exports.

Docs: https://github.com/webpro/knip/blob/next/README.md. Knip v1 is almost out, this PR holds v1.0.0-beta.9 but the API shouldn't change anymore, so that should not affect this PR.

Some remarks for now:

  • In the dependency department, there are still a few false positives that will sort themselves out with new plugins for vitest and npm-package-json-lint
  • It does look like yargs is unused and can be removed?
  • Seems like knip and ts-prune agree about there being no unused exports... Obviously it's up to you what to do with this.

Would you be interested at all in having Knip in this repo? Happy to answer any questions or feedback you might have.

Fixes #192

.eslintrc.cjs Outdated
@@ -12,7 +12,7 @@ module.exports = {
overrides: [
{
extends: ["plugin:markdown/recommended"],
files: ["**/*.{md}"],
files: ["**/*.md"],
Copy link
Owner

Choose a reason for hiding this comment

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

Heh, nice catch. If you can pull this out into a separate PR I'd be happy to pull it in quickly.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, this is probably what's causing https://github.com/JoshuaKGoldberg/template-typescript-node-package/actions/runs/3943867603/jobs/6749189690:

/home/runner/work/template-typescript-node-package/template-typescript-node-package/README.md
  96:1  error  Parsing error: ESLint was configured to run on `<tsconfigRootDir>/README.md/2_2.ts` using `parserOptions.project`: <tsconfigRootDir>/tsconfig.eslint.json

Should probably split this out into a separate issue & PR. I can take a look at this too if you don't have time. ❤️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's convenient to leave it in this PR? Eventually it's caused by different backing glob parsers (in ESLint and Knip), but necessary for green lights in this PR.

@codecov-commenter
Copy link

codecov-commenter commented Jan 8, 2023

Codecov Report

Merging #174 (d3da106) into main (f25d5a8) will not change coverage.
The diff coverage is n/a.

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

@@            Coverage Diff            @@
##              main      #174   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines            6         6           
  Branches         3         3           
=========================================
  Hits             6         6           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Hey again @webpro! Nice to see a tool getting worked on that can holistically tackle unused code/dependency detection. I'm up for trying it out! Especially since it's already spotted yargs as an unnecessary dependency. 😄

But, a few blockers:

  • There shouldn't be any false positives
  • Let's get it running in CI with a GitHub Action
  • You'll want to remove ts-prune in this PR too, since it'll now be redundant
  • The dependency on an explicit esbuild version

pnpm-lock.yaml Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@webpro
Copy link
Contributor Author

webpro commented Jan 9, 2023

Hey again @webpro! Nice to see a tool getting worked on that can holistically tackle unused code/dependency detection. I'm up for trying it out! Especially since it's already spotted yargs as an unnecessary dependency. 😄

Great, happy to hear you are interested!

But, a few blockers:

  • There shouldn't be any false positives
  • Let's get it running in CI with a GitHub Action
  • You'll want to remove ts-prune in this PR too, since it'll now be redundant
  • The dependency on an explicit esbuild version

Things are sorted out, except for the esbuild dependency, since Knip needs to require/import configuration files.

@webpro
Copy link
Contributor Author

webpro commented Jan 9, 2023

Just saw that markdownlint is now also unused as Knip also requires a custom dependency resolver to find it in extends.

Also enabled production mode:

$ knip --production
Unused exports in namespaces (1)
greet  src/greet.ts

If interested, read more at https://github.com/webpro/knip/tree/next#production-mode

Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Ok this is looking great, thanks @webpro! I looked at this more closely and think this is the last list of things to request:

  • ts-prune is still referenced in the README.md. It should be replaced with knip.
    • Would you like the honors of writing the marketing text for it? 😁
  • knip.jsonc references the setup/ directory. But the setup script removes that directory. The setup script should be augmented to change knip.jsonc to no longer reference setup/

Do you have time to make these changes? Absolutely no worries if not, I should Soon ™️ 🙂

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label Jan 17, 2023
@webpro
Copy link
Contributor Author

webpro commented Jan 18, 2023

Ok this is looking great, thanks @webpro! I looked at this more closely and think this is the last list of things to request:

  • ts-prune is still referenced in the README.md. It should be replaced with knip.

    • Would you like the honors of writing the marketing text for it? 😁

Done :)

  • knip.jsonc references the setup/ directory. But the setup script removes that directory. The setup script should be augmented to change knip.jsonc to no longer reference setup/

The entry files reference the two script/setup*.js files, is that not correct?

Do you have time to make these changes? Absolutely no worries if not, I should Soon ™️ 🙂

Sure, just pushed the changes.

@JoshuaKGoldberg
Copy link
Owner

The entry files reference the two script/setup*.js files, is that not correct?

...sort of!

In more detail: the repo contains a script/ folder that includes a setup.js file. That file is meant to be run (pnpm run setup) when someone forks the template. "Setup" here refers to assorted repo tasks such as renaming to their repo name, populating GitHub labels, etc.

The setup script is also meant to remove the setup/ directory and any references to setup scripts. Which it should because there's no need to keep that directory around once a repo has been set up! So, anything referring to the setup/ directory should be removed as a part of script/setup.js

Does that all sound reasonable to you? (This repo is still new and I'm still learning how to document+explain it!)

@webpro
Copy link
Contributor Author

webpro commented Jan 19, 2023

Ah, that makes sense. I've now actually ran the setup. You may want to remove the chalk dependency then, too, Knip thinks it's no longer in use after running the setup :)

Ideally, the script entries in knip.jsonc are removed during setup? So that the setup*.js files are still linted before that.

@JoshuaKGoldberg
Copy link
Owner

remove the chalk dependency then, too

Another win for Knip! 😄 #193

Ideally, the script entries in knip.jsonc are removed during setup? So that the setup*.js files are still linted before that.

Yes!

@webpro
Copy link
Contributor Author

webpro commented Jan 19, 2023

Alright, I prefer to leave the rest of the project-specific things and scripts to you. Let's get this PR merged?

@JoshuaKGoldberg
Copy link
Owner

Gif of Selena Gomez saying Let's Do It

@JoshuaKGoldberg JoshuaKGoldberg removed the status: waiting for author Needs an action taken by the original poster label Jan 20, 2023
Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🚀

@JoshuaKGoldberg JoshuaKGoldberg changed the title chore: introduce knip feat: introduce knip Jan 20, 2023
@JoshuaKGoldberg JoshuaKGoldberg changed the title feat: introduce knip feat: replace ts-prune with knip Jan 20, 2023
@JoshuaKGoldberg JoshuaKGoldberg merged commit c652bda into JoshuaKGoldberg:main Jan 20, 2023
@webpro
Copy link
Contributor Author

webpro commented Jan 22, 2023

Nice!

@webpro webpro deleted the feature/introduce-knip branch January 22, 2023 02:24
@JoshuaKGoldberg
Copy link
Owner

@allcontributors add @webpro for feature tooling

@allcontributors
Copy link
Contributor

@JoshuaKGoldberg

I've put up a pull request to add @webpro! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Replace ts-prune with Knip
3 participants