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

Knip output differs depending on the presence of dist #459

Closed
alecmev opened this issue Jan 18, 2024 · 6 comments
Closed

Knip output differs depending on the presence of dist #459

alecmev opened this issue Jan 18, 2024 · 6 comments
Labels
regression Something is no longer working

Comments

@alecmev
Copy link

alecmev commented Jan 18, 2024

Hi! I finally updated Knip from 2.41.5 to 4.0.3 and it told me to move some type-only dependencies from devDependencies to dependencies, which I did (this is desired). Then I pushed these changes and Knip failed in CI, asking me to do the exact opposite of the above. The only difference is the absence of dist directories, as we don't build before static checks.

Made a repro. Run npm run test and observe this:

Unused dependencies (1)
@org/foo  packages/bar/package.json

Then run npm run build and npm run test again, no error.

Seems like some sort of composite project issue? Not sure.

@alecmev alecmev added the regression Something is no longer working label Jan 18, 2024
@webpro
Copy link
Collaborator

webpro commented Jan 19, 2024

Quoting from https://knip.dev/features/production-mode#strict-mode:

  • Type-only imports should be in devDependencies

Unfortunately there's no clear-cut solution for this yet.

Related to #248.

@alecmev
Copy link
Author

alecmev commented Jan 19, 2024

To clarify, I don't feel that strongly about whether it should be in dependencies or devDependencies, I understand that it's an ecosystem problem, but I do want Knip to be consistent about it 😛 In this case, Knip wants type-only deps to be in devDependencies if there is no dist, and in dependencies if there is a dist, which seems to be a bug.

@webpro
Copy link
Collaborator

webpro commented Feb 8, 2024

It's a bit of double-edged sword: adding dist/index.js will include it in the program, since it's being referred to.

(This is how the TypeScript backend works w/ ts.createProgram()).

The correct result in this case seems to be that there are no unused dependencies. There's no way for Knip to understand that e.g. /dist/index.js should map to src/index.ts (regardless of the presence of the former). I understand the annoyance, but at this point there isn't much I can do about it. That's why I'm going to close this issue.

Related/similar: #472 (comment)

@webpro webpro closed this as not planned Won't fix, can't repro, duplicate, stale Feb 8, 2024
@alecmev
Copy link
Author

alecmev commented Feb 8, 2024

Is dist included in the repro I posted though? It’s both specified as an outDir in tsconfig.json and listed in .gitignore, I would expect Knip to ignore it, or is that assumption wrong?

@webpro
Copy link
Collaborator

webpro commented Feb 8, 2024

Knip ignores it, but the TypeScript backend does not.

@alecmev
Copy link
Author

alecmev commented Feb 8, 2024

Just in case, this happens only in a monorepo. When I was trying to come up with a repro, I couldn't get it with just a simple plain package. If you go to the same repro above, do an npm run build, cd packages/bar and run npx knip --production --strict, you'll get the desired output (ignore "unused files", it's due to the lack of a local .gitignore):

Unused dependencies (1)
@org/foo  package.json

Then do a cd ../.. and run npx knip --production --strict again, no errors. There must be more to this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Something is no longer working
Projects
None yet
Development

No branches or pull requests

2 participants