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

Introduce Knip & remove unused code & dependencies #7545

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

webpro
Copy link
Contributor

@webpro webpro commented Jan 22, 2025

This repository contains quite some unused code and dependencies, so this PR is large. And I'm not sure how to split it up in smaller chunks, but the good news is that we can replay and execute the process at any time. This is reflected in the separate commits.

Knip is a CLI tool to find & remove unused files, exports and dependencies. Recently plugins for Expo and Metro were added so I figured why not apply Knip to the open source app I'm using myself daily. Turns out, it seems to do a pretty decent job here.

There's a few commits to add Knip, some configuration and make some fairly small changes to make Knip run without any issues reported: no unused files, no unused exports and no unlisted or unused dependencies. So the first ~6 commits are the basis to start the process of actually removing things.

Then all commits that start with yarn lint-unused and yarn rm-unused do all the hard work of actually finding and removing things:

  • yarn lint-unused runs Knip, add the --fix and --allow-remove-files flags at will.
  • yarn rm-unused runs the internal linter (eslint) and passes this on to remove-unused-vars, a little tool I made as I found that eslint --fix doesn't remove all unused variables. The commit "Add remove-unused-vars" contains this tool to automate the tedious job of removing unused variables within a file (Knip only removes unused exports), feel free to eject. I found it to work amusingly well on this particular codebase, though.

The commands can be ran repeatedly without issues. I did this until no more code got removed.

What's not in this PR yet is adding a workflow to run Knip as part of CI.

Since the process is automated, we could come back to this after future changes as well, in case this isn't the right time. I understand this might come across as overwhelming. Feel free to reject the PR entirely if this is just too large of a change, no questions asked.

The following has been done to verify everything's still functional:

  • yarn test
  • yarn build-web
  • yarn web and navigate around locally

Feel free to ask any questions you may have, happy to collaborate to make this round of housekeeping happen :)

@gaearon
Copy link
Collaborator

gaearon commented Jan 22, 2025

Hi! Appreciate the intent but I don't think we'd get a significant value out of this — when feature development is fast, code can churn quickly between being used and being dead, and I don't think we'd want to constantly flip-flop the declarations etc.

@webpro
Copy link
Contributor Author

webpro commented Jan 22, 2025

Fair enough. We could also look into areas to include or exclude for such feature development, and still benefit from the automated find & removals for the rest of the codebase, including unlisted and unused dependencies for each of the workspaces.

@webpro webpro marked this pull request as ready for review January 22, 2025 20:24
@webpro
Copy link
Contributor Author

webpro commented Jan 25, 2025

For the record, there's also knip --dependencies to focus on package management solely (and thus ignore the --files and --exports parts).

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.

2 participants