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

chore(tooling): install knip and remove unused files / dependencies #3106

Open
wants to merge 7 commits into
base: next
Choose a base branch
from

Conversation

jeanpierrecarvalho
Copy link
Contributor

chore(tooling): install knip and remove unused files / dependencies

Changes Made:

  • Installed knip to analyze unused files and dependencies
  • Removed unused files and dependencies identified by knip
  • Add some folders to be ignored by knip

Details of Changes:

  • Added knip to the project as a dev dependency to improve project cleanliness and remove dead code
  • Ran an analysis using knip and removed files that were no longer in use or required by the project
  • Updated related configurations and documentation to reflect the changes

Additional Details:

  • These changes help streamline the project and reduce clutter, improving overall maintainability
  • No functional changes were introduced; the focus was solely on project hygiene and code cleanup

What is next?

  • pnpm knip can run in the pre-commit hook OR / AND
  • can be added to GHA on PR creation OR / AND
  • can be added to release process and tag creation

@jeanpierrecarvalho jeanpierrecarvalho requested a review from a team as a code owner September 11, 2024 16:45
Copy link

netlify bot commented Sep 11, 2024

Deploy Preview for fakerjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 7f2b642
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/66e2fdc208f09f000873ec85
😎 Deploy Preview https://deploy-preview-3106.fakerjs.dev
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.97%. Comparing base (18ab2c7) to head (7f2b642).
Report is 16 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3106      +/-   ##
==========================================
+ Coverage   99.96%   99.97%   +0.01%     
==========================================
  Files        2776     2776              
  Lines      226260   226260              
  Branches      945      592     -353     
==========================================
+ Hits       226183   226206      +23     
+ Misses         77       54      -23     

see 1 file with indirect coverage changes

package.json Show resolved Hide resolved
knip.config.ts Outdated Show resolved Hide resolved
knip.config.ts Outdated Show resolved Hide resolved
@@ -94,12 +95,6 @@
],
"devDependencies": {
"@actions/github": "6.0.0",
"@algolia/client-search": "5.2.3",
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this dependency is now, optional. It would be good if somebody could test that as well.

Thinking: I wonder when that change happened.

package.json Outdated Show resolved Hide resolved
@@ -57,7 +57,7 @@
},
{
"groupName": "doc-dependencies",
"matchPackageNames": ["@algolia/client-search", "ts-morph", "vitepress"]
"matchPackageNames": ["ts-morph", "vitepress"]
Copy link
Member

Choose a reason for hiding this comment

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

We should probably add vue/vue-tsc/@vueuse/core in here too. (Different PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will create a new PR for that 🙏🏻

@@ -30,7 +30,8 @@
"docs:test:e2e:open": "run-p --race docs:serve \"cypress open\"",
"release": "commit-and-tag-version",
"prepublishOnly": "pnpm run clean && pnpm install && pnpm run build",
"preflight": "pnpm install && run-s generate format lint build test:update-snapshots ts-check"
"preflight": "pnpm install && run-s generate format lint build test:update-snapshots ts-check",
"knip": "knip"
Copy link
Member

Choose a reason for hiding this comment

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

If this something we should run as part of preflight? Or in our CI to find these as early as possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

Preflight is already slow enough 😀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just ran the preflight and yes, it's painful ahhahahah Do we need to do this preflight like that? Why the process is not splited using git hooks e.g and run, only if needed. We can perform some tasks only in the files in stage, using lint-staged e.g, WDYT? I can draw a proposal and explain my pov better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

i think it would be good to open a separate discussion on ways to speed up preflight

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! I will create that discussion and add my thought there 🙏🏻

@ST-DDT ST-DDT added c: chore PR that doesn't affect the runtime behavior p: 1-normal Nothing urgent c: dependencies Pull requests that adds/updates a dependency s: needs decision Needs team/maintainer decision c: infra Changes to our infrastructure or project setup labels Sep 12, 2024
@ST-DDT ST-DDT added this to the vAnytime milestone Sep 12, 2024
@ST-DDT
Copy link
Member

ST-DDT commented Sep 12, 2024

Team Decision

For now, we don't see a need to keep knip permanently in the project.
But we would like to have the one time cleanup anyway.

Do you know how good the github ci integration of knip is?
aka does it show the errors only in the build logs or also as error markers in the PullRequest?

We will discuss this some more in the next team-meeting with other team members.

@ST-DDT ST-DDT mentioned this pull request Sep 14, 2024
1 task
@ST-DDT ST-DDT added the needs rebase There is a merge conflict label Sep 16, 2024
@@ -94,12 +95,6 @@
],
"devDependencies": {
"@actions/github": "6.0.0",
"@algolia/client-search": "5.2.3",
Copy link
Member

Choose a reason for hiding this comment

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

Please create a separate PR that remove this dependency. That way it is easier to test for breaking changes.

@@ -132,7 +127,6 @@
"typescript": "5.5.4",
"typescript-eslint": "8.3.0",
"validator": "13.12.0",
"vite": "5.4.2",
Copy link
Member

Choose a reason for hiding this comment

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

Please create a separate PR that remove this dependency. That way it is easier to test for breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: chore PR that doesn't affect the runtime behavior c: dependencies Pull requests that adds/updates a dependency c: infra Changes to our infrastructure or project setup needs rebase There is a merge conflict p: 1-normal Nothing urgent s: needs decision Needs team/maintainer decision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants