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: add knip #8192

Merged
merged 50 commits into from
May 13, 2024
Merged

chore: add knip #8192

merged 50 commits into from
May 13, 2024

Conversation

auvred
Copy link
Member

@auvred auvred commented Jan 5, 2024

PR Checklist

Overview

TODO:

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @auvred!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlify bot commented Jan 5, 2024

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 2188ef8
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/66422ee784e82a0008abd70c
😎 Deploy Preview https://deploy-preview-8192--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 98 (🟢 up 2 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

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

@@ -1,5 +1,9 @@
'use strict';

// @ts-check
Copy link
Member

Choose a reason for hiding this comment

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

[Praise] Nice, +1 to getting // @ts-check in here.

This reverts commit 36fa481.
Copy link
Member

@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.

✂️ this is fantastic, nice job @auvred! Love the explanations throughout and all the removed stuff.

Nothing I commented is a blocker IMO. They can all be followup issues IMO.

knip.ts Show resolved Hide resolved
knip.ts Outdated
Comment on lines 63 to 65
'packages/repo-tools': {
ignoreDependencies: ['tsconfig.json'],
},
Copy link
Member

Choose a reason for hiding this comment

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

Yeah this feels like a feature request for Knip: shouldn't it understand that npx tsc goes to TypeScript?

...separately, -p defaults to tsconfig.json. yarn knip and yarn typecheck pass happily for me when I remove the -p tsconfig.json altogether. Maybe a good reason to remove them?

knip.ts Show resolved Hide resolved
knip.ts Outdated Show resolved Hide resolved
'src/mock/lru-cache.js',
'src/mock/path.js',
'src/mock/typescript.js',
'src/mock/util.js',
Copy link
Member

Choose a reason for hiding this comment

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

You know, as a followup I bet we could switch these over to plain .ts files, and remove the need for the requireResolved that's tripping up Knip... Not blocking at all IMO.

@@ -1,4 +1,5 @@
export * from './applyDefault';
export * from './context';
Copy link
Member

Choose a reason for hiding this comment

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

Probably yeah. The fact that nobody has reported anything makes me less motivated to - but, yeah, we should.

Co-authored-by: Josh Goldberg ✨ <[email protected]>
@@ -13,16 +13,21 @@
"lint": "npx nx lint",
"postinstall-script": "npx tsx ./src/postinstall.mts",
"test": "npx jest --coverage",
"typecheck": "npx tsc -p tsconfig.json --noEmit"
"typecheck": "npx tsc --noEmit"

This comment was marked as resolved.

This comment was marked as resolved.

Copy link
Member

@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.

🚀

@webpro
Copy link

webpro commented Apr 29, 2024

Hey folks, this is great! I didn't check out the details of this PR, but feel free to drag me into this, love to help out a bit here. If there's feature requests or other annoying stuff in/from Knip I'd love to hear about it.

@auvred
Copy link
Member Author

auvred commented Apr 30, 2024

Integration tests failures are unrelated. See #9029 (comment):

8127873 / #8640

This commit changed the context.report from { node, ... } to { loc, ... } Hence the nodeType is no longer visible in the output as the rule is not reporting against the node.

I think we can finally merge this PR!

UPD: this issue has been resolved on main

@auvred
Copy link
Member Author

auvred commented May 12, 2024

Unfortunately, merging main two weeks ago automatically dismissed an existing approval, so I can't merge this PR (the PR authors can't approve their PR). Could someone please re-review and stamp this PR so we can merge it?

Every Monday, after the automatic release, there are a few merge conflicts due to bumped @typescript-eslint/* versions, and it's a bit tedious to resolve them every Monday 😄

@rubiesonthesky
Copy link
Contributor

@auvred Could you try running yarn dedupe in this branch and see if all the tests still pass? I think you may have fixed the problem that all the renovate branches have. But I suspect, that it will only manifest itself after yarn dedupe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Forwarding #8192 (comment) here:

@rubiesonthesky, I've already ran yarn dedupe in one of the previous commits on this PR, but the ast-spec tests didn't pass with deduped lockfile:

https://github.com/typescript-eslint/typescript-eslint/actions/runs/8804176361/job/24164146899

So, I reverted these changes in 5c56b50


Might be useful info: Joshua ran yarn dedupe in the #9002 (merged in v8 branch) - #9002 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

@auvred Sorry and thank you :) That explains everything.

Copy link
Member Author

Choose a reason for hiding this comment

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

NP! Thank you for Renovate issue investigation!

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think once someone takes the time to fix the test errors in #8589, we could probably keep it deduped forever. Which would be great.

Copy link
Member

@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.

💯 looks fantastic, thanks for pressing on this! Very excited to have the cross-file unused code detection.

Marking as 1 approval to give you a chance to merge whenever you want. 😁

@JoshuaKGoldberg JoshuaKGoldberg added the 1 approval One team member has approved this PR; a second should be enough to merge it label May 13, 2024
@auvred auvred merged commit f53fece into typescript-eslint:main May 13, 2024
61 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 approval One team member has approved this PR; a second should be enough to merge it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repo: Add Knip
5 participants