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 does not resolve package.json exports quite correctly #853

Open
6 tasks done
maastrich opened this issue Nov 25, 2024 · 5 comments
Open
6 tasks done

🐛 Knip does not resolve package.json exports quite correctly #853

maastrich opened this issue Nov 25, 2024 · 5 comments
Labels
bug Something isn't working

Comments

@maastrich
Copy link

maastrich commented Nov 25, 2024

Prerequisites

Reproduction url

maastrich/repro#2

Reproduction access

  • I've made sure the reproduction is publicly accessible

Description of the issue

First, thanks for the amazing tool, it's really impressive how quick and effective knip is 👏

When setting up exports using glob syntax in the package.json, typescript can resolve * to be either a path part or multiple path part:

"./*": "./src/public/*/index.ts" // can resolve to
   -> "./src/public/foo/bar/index.ts"
   -> "./src/public/baz/index.ts"

but knip doesn't

For the moment I've found a workaround where I add this in my package.json so knip can resolve le path
(leaving both as typescript resolve the first one and knip the second)

"./*": "./src/public/*/index.ts",
+ "./**": "./src/public/**/index.ts",
@maastrich maastrich added the bug Something isn't working label Nov 25, 2024
@webpro
Copy link
Collaborator

webpro commented Nov 26, 2024

Thanks for report. That's correct according to the Node.js docs and honestly I'm a bit baffled by it:

* maps expose nested subpaths as it is a string replacement syntax only.

All instances of * on the right hand side will then be replaced with this value, including if it contains any / separators.

From a Knip perspective, the issues I have with this:

  • a lot more files might become entry files and this isn't great in terms of finding unused exports (which aren't reported by default in entry files)
  • unsure about the performance penalty, Knip has to traverse the fs and find all matching files (a runtime does string replacement first and then starts resolution).
  • people who'd be baffled like me might unintentionally use * thinking it actually means * in glob pattern matching, with subtly unexpected results both in exposed exported paths as in Knip analysis.

What about this workaround, would this work:

"./*": "./src/public/**/index.ts",

Just curious, not saying it's a solution.

@maastrich
Copy link
Author

Unfortunately the workaround you proposed is not working with rollup (but typescript handles it)
I was more thinking about an option in the knip config as a "quick fix" saying

pkgJsonExport: "glob" | "node";

where glob is the default (to remain non-breaking) and would become node in a future major ?

@webpro
Copy link
Collaborator

webpro commented Nov 26, 2024

Just saying, the generic (and intended) solution to this is to add entry patterns to your knip config manually:

{
  "entry": ["./src/public/{a,b}/index.ts"]
}

At this point this feels better than rewriting * to globstar ** and marking "everything" as an entry file (the node option), even though it goes 100% against the "zero-config aim".

@maastrich
Copy link
Author

I'll have to try it, but I think that next major i there is one should handle exports as specified in node.js docs

@webpro
Copy link
Collaborator

webpro commented Nov 27, 2024

Understood, but this is a good example of static analysis versus runtime behavior and there are tradeoffs to be considered.

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