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: make knip understand yarn pnp #924

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

Conversation

jwoo0122
Copy link
Contributor

@jwoo0122 jwoo0122 commented Jan 22, 2025

Make knip understand yarn pnp. It can now look into manifest files inside yarn cache, and correctly check peer dependency or bin field.

Maybe we can make this feature to be enabled with yarn plugin made from #864 but I couldn't find how to patch manifest helpers with plugin.

You should see manifest/helpers.ts.

Copy link

pkg-pr-new bot commented Jan 22, 2025

Open in Stackblitz

npm i https://pkg.pr.new/knip@924

commit: 5e2b43f

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 made a real yarn workspace to test real pnp api, but it'll be good to see any ideas for this fixture.

@jwoo0122

This comment was marked as resolved.

@jwoo0122 jwoo0122 marked this pull request as draft January 22, 2025 16:28
@jwoo0122 jwoo0122 changed the title feat: make bun understand yarn pnp feat: make knip understand yarn pnp Feb 6, 2025
Comment on lines +14 to +37
const findNearestPnPFile = (startDir: string) => {
// Find the nearest .pnp.cjs file by traversing up
let currentDir = startDir;
while (currentDir !== '/') {
const pnpPath = join(currentDir, '.pnp.cjs');
if (isFile(pnpPath)) {
const pnpApi = _require(pnpPath);
pnpApi.setup();
pnpStatus.dir = startDir;
pnpStatus.pnpPath = pnpPath;
pnpStatus.enabled = true;
return;
}
// Move up one directory
const parentDir = dirname(currentDir);
if (parentDir === currentDir) {
break; // Reached root
}
currentDir = parentDir;
}
pnpStatus.dir = startDir;
pnpStatus.pnpPath = '';
pnpStatus.enabled = false;
};
Copy link
Contributor Author

@jwoo0122 jwoo0122 Feb 8, 2025

Choose a reason for hiding this comment

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

I made this by assuming that knip needs to be able to understand yarn pnp, even if it isn't under yarn pnp environment, e.g yarn dlx knip. If it's not, we can just simply import pnpapi directly, instead of manually finding it.

@jwoo0122 jwoo0122 marked this pull request as ready for review February 8, 2025 10:57
@webpro
Copy link
Collaborator

webpro commented Feb 14, 2025

Thanks for the PR, @jwoo0122. Without knowing exactly how PnP works, please let my try to understand what support for PnP means to Knip, and what features (i.e. issue types) are still (un)available/relevant.

Some initial questions that pop up in my mind:

  • The fixture has only "internal" packages and ignores a bunch of stuff, but perhaps we could see a more realistic example?
  • Is there support for monorepos/workspaces (i.e. package.json#workspaces)? Or is that not relevant cq nothing to worry about?
  • Since there are no node_modules anymore, what about:
    • Binaries (node_modules/.bin) e.g. those referenced in package.json#scripts
    • Knip reads package.json#types etc. to see if types are included with the package (so e.g. @​types/eslint is unused if eslint is listed).

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