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

🐛 inconsistent behavior of unused exports with --fix flag #874

Open
6 tasks done
yishuolin opened this issue Dec 9, 2024 · 2 comments
Open
6 tasks done

🐛 inconsistent behavior of unused exports with --fix flag #874

yishuolin opened this issue Dec 9, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@yishuolin
Copy link
Contributor

Prerequisites

Reproduction url

https://stackblitz.com/edit/knip-fix-example-y2fu4h2n?file=re-export.js&view=editor

Reproduction access

  • I've made sure the reproduction is publicly accessible

Description of the issue

Hi!
I’ve encountered an issue related to the detection of unused exports.

Description

When running Knip on the following code without the --fix flag, no issues are reported:

import { A } from './export.js';
A;
export { A } from './export.js';

However, when running Knip with the --fix flag, it correctly detects that A is an unused export.

Expected Behavior

Knip should identify unused exports, regardless of whether the --fix flag is used.

Potential Fix

While reviewing the codebase, I noticed the following line in the collectUnusedExports function:

if (!isFix && exportedItem.isReExport) continue;

Link to the line of code

It seems that re-exports are being excluded unless the --fix flag is used. Maybe we need to adjust the logic to ensure that re-exports are not excluded during the collection of unused exports?

@yishuolin yishuolin added the bug Something isn't working label Dec 9, 2024
@webpro
Copy link
Collaborator

webpro commented Dec 9, 2024

In this small example it may look a bit silly, but the logic is for larger reports where it's not convenient to have duplicates (i.e. re-exports) reported of essentially the same export.

The fix mode removes exports including re-exports, and that's what you're seeing in this minimal example. Note that the includeEntryExports flag is of influence here as well.

While I agree this might be confusing at times, I think the current behavior makes sense. Would you agree? Maybe there are ways we could improve? With better output, or in docs perhaps.

@yishuolin
Copy link
Contributor Author

Thank you for the explanation. To simplify things, I’ve created a refined reproduction that focuses solely on the issue we discussed (without the includeEntryExports flag).

I agree that avoiding duplicate re-exports in reports is convenient for larger projects. However, IMHO, I still think the behavior should be consistent, and the detailed reports are acceptable—similar to how users expect many issues to be reported when running Knip for the first time.

That said, refining the documentation to clarify this behavior would also work for me.

Would it also make sense to introduce a flag to control whether re-exports are ignored?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants