Skip to content

Conversation

@nickrttn
Copy link
Contributor

This adds support for the file-based routing Vite/RsPack/RsBuild/Webpack plugin of @tanstack/react-router. The only reliable way I saw to parse the config files of those tools to generate the correct entries was through building an AST using TS.

I saw you've got some helpers for TS AST's in the codebase. Let me know if I can/need to use any of those.

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 26, 2024

Open in Stackblitz

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

commit: 8392b38

@webpro
Copy link
Member

webpro commented Nov 26, 2024

Thanks for the PR, @nickrttn!

This adds support for the file-based routing Vite/RsPack/RsBuild/Webpack plugin of @tanstack/react-router. The only reliable way I saw to parse the config files of those tools to generate the correct entries was through building an AST using TS.

I see what you're doing here, why it's needed, and I find it nice you gave it a shot and impressive you pulled it off. Now to me, the question is whether the complexity of this plugin warrants users not having to also configure Knip, like so:

{
  "production": ["src/path/to/routeTree.gen.ts"],       // either one
  "tanstack-router": {
    "production": ["src/path/to/routeTree.gen.ts"],     // of these is fine
  }
}

How often generatedRouteTree is modified is relevant, I have no idea. I think the value routesDirectory is less important to find as route files are picked up through src/main.ts anyway. Maybe depending on the src/main.tsx is even preferable here as that file contains actual usage, and we don't need to !negate ignore patterns, right?

So far, I think none of the plugins use or need ts at all, so currently I'm a bit on the fence with this one. And perhaps I'm missing something as well. WDYT? Eager to hear what your thoughts are on this one.

@nickrttn
Copy link
Contributor Author

nickrttn commented Dec 2, 2024

@webpro just wanted to let you know that this hasn't dropped off my radar, will get back to this as soon as I can!

@webpro
Copy link
Member

webpro commented Dec 2, 2024

That's great, no rush.

@webpro
Copy link
Member

webpro commented Mar 24, 2025

@nickrttn I've been working on support for a new resolveFromAST in plugins so we can implement functionality like in this PR. I'm going to update things better to clarify what's going on, but at least you know I'm working on this. Feel free to check it out, hack on it, provide feedback: #1005. There's also the pkg.pr.new publish so you could try it out.

@webpro
Copy link
Member

webpro commented Apr 8, 2025

Closing this in favor of #1005.

@webpro webpro closed this Apr 8, 2025
@webpro
Copy link
Member

webpro commented Apr 24, 2025

@nickrttn Is there any chance you could explain why you submitted the plugin? It seems like the generated routes tree file (e.g. src/routeTree.gen.ts) always has to be imported by the user (e.g. from src/index.ts) which is already included in the analysis. From that, it looks like the plugin wouldn't be necessary. But I might be missing something and you might have a use case justifying the plugin. If we can't find a good reason to keep it we should remove the plugin.

@webpro
Copy link
Member

webpro commented Apr 24, 2025

cc @brandongit2 and @codepunkt

@nickrttn
Copy link
Contributor Author

@webpro it's been a while, but if I recall correctly the project I worked on at the time had a reason for git-ignoring the routeTree.gen.ts, could that be it?

@nickrttn
Copy link
Contributor Author

My apologies for letting this die a silent death btw, my priorities changed and some real life stuff happened around the same time. Was hectic for a bit.

@webpro
Copy link
Member

webpro commented Apr 24, 2025

@webpro it's been a while, but if I recall correctly the project I worked on at the time had a reason for git-ignoring the routeTree.gen.ts, could that be it?

That could definitely be it! Then I 100% understand. Also, it's not uncommon for build artifacts to be on disk before knip can run successfully (another example is https://knip.dev/features/source-mapping).

I'll give it another thought, but it's a good argument you got there to keep it. But I'm still leaning towards removing it, although it's not ideal for end-users and probably requires some warnings/docs.

My apologies for letting this die a silent death btw, my priorities changed and some real life stuff happened around the same time. Was hectic for a bit.

No worries! Thanks for helping out. It has been super useful to create the resolveFromAST feature Knip has now. Hope you're well.

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