Repository navigation
feat(solidjs-vite-plugin): read lazily loaded plugin and ternary paths - #2147
everton-dgn wants to merge 10 commits into
Conversation
Mark the files @solidjs/vite-plugin loads in start mode as production
entries: src/entry-server.*, src/entry-client.*, src/App.*,
src/Document.*, env.{ts,js} and the module paths in the start options.
Resolve start entries from the Vite root, accounting for SSR and client
mode. Also include serverFunctions.configure with or without start.
Keep start mode conventions, literal start.* paths, serverFunctions.configure and the Vite root for configs that import the plugin statically. Lazily loaded plugins, ternary paths and the SSR and client mode rules move to follow-up pull requests. The tests are now one table over smaller fixtures without node_modules stubs.
commit: |
…y-paths # Conflicts: # packages/knip/fixtures/plugins/solidjs-vite-plugin-entries/vite.config.ts # packages/knip/src/plugins/solidjs-vite-plugin/resolveFromAST.ts # packages/knip/test/plugins/solidjs-vite-plugin.test.ts
webpro
left a comment
There was a problem hiding this comment.
Thanks for splitting this out. The failed Node 20 workflow is a runner crash: shard 3/3 passed 460/460 tests and the later knip --production --strict step exited with -1073741502, after which fail-fast cancelled the other jobs. #2148 is green on the same workflow.
| return false; | ||
| }; | ||
|
|
||
| const getPluginCalls = (program: Program) => { |
There was a problem hiding this comment.
Reading every call in the config when the plugin is not statically imported means any call whose first argument has a start property is read as the Solid plugin call, as the description notes. If we keep it, could it only kick in when the config contains import('@solidjs/vite-plugin')? That covers the lazy pattern from the description and keeps other configs on the static path.
There was a problem hiding this comment.
Done: the fallback now only applies when the config contains import('@solidjs/vite-plugin'), detected with an ImportExpression visitor. Without it, findImportedCalls stays the only path. The type-only import check is gone, since a type import never produces a callable binding.
…gin-lazy-paths # Conflicts: # packages/knip/fixtures/plugins/solidjs-vite-plugin-spa/vite.config.ts # packages/knip/fixtures/plugins/solidjs-vite-plugin/vite.config.ts # packages/knip/src/plugins/solidjs-vite-plugin/resolveFromAST.ts # packages/knip/test/plugins/solidjs-vite-plugin.test.ts
|
Thanks for all the work. Really appreciate all your contributions. Upon closer look, are you sure this is an actual use case? Dynamic imports are not that common? In general, this is the downside of So That's why I see two paths forward, please pick one:
Let me know if you think one of the options is feasible. And sometimes the additional complexity just isn't worth it to get rid of one false positive. |
|
Thanks! It came from a real app, but I agree it isn't common enough to chase in So going with option 1 and closing this one. #2148 is updated on top of main without the lazy import, ternary and spread handling. Configs like the one above can list their entries in the Knip config. |
|
Thank you for thinking along with me, excellent stuff. |
Follow-up to #2144, which only reads calls to a statically imported
@solidjs/vite-plugin. Configs that load their Vite plugins withimport()get no entries yet:What changes:
import('@solidjs/vite-plugin'), the first argument of every call in the config is read, since the plugin binding can't be traced through the lazy moduleundefined,true) keeps the convention; forenv, onlyfalsedrops itWithout that dynamic import, the calls that get read stay the same: only direct calls to the statically imported binding. A plugin imported through a local module is not followed, and a
timing({ start })call next to it is not read; the-spafixture covers that and fails without the gate. The ternary handling applies there too. In a lazy config, another call with astartorserverFunctionsobject would be read too, and options passed as a variable (solid(options)) are not read.I ran this on a Solid 2 app that loads its Vite plugins like the config above and listed six of these files as entries by hand. Without those entries, Knip 6.41.0 reported 5 unused files and 1 unused export on top of the baseline. With this change the report matches the baseline.