feat: Add plugin for React router 7 framework mode #931
Conversation
Thanks! Good stuff indeed 👍
Do you think we might get in trouble if library mode will be supported here? Can we distinguish that easily, or would we need separate plugins, if at all necessary?
This looks fine. Maybe you know of a public project we could run this on for some additional smoke testing? |
| if (existsSync(routesPathTs)) { | ||
| routeConfig = await load(routesPathTs); | ||
| } else if (existsSync(routesPathJs)) { | ||
| routeConfig = await load(routesPathJs); |
There was a problem hiding this comment.
maybe good idea to check if routeConfig is actually set later on?
There was a problem hiding this comment.
I initialize routeConfig as an empty array above. So if no routes file is defined it is ignored.
| file: "routes/layout.tsx", | ||
| children: [{ file: "routes/another-route.tsx" }], | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
seeing things in the docs like route("/", "./home.tsx") - do we cater for that as well? or how's that returned when loading app/routes.ts?
There was a problem hiding this comment.
route("/", "./home.tsx") returns a simple object containing { file: "./home.tsx", path: "/" }. Since we are joining the path like this: join(appDir, route.file) and the routes.ts file is defined in appDir this is catered for.
Do you mean if library mode wil be supported by
Yes. We can test it on The epic stack which uses I have a public project which uses react router flat routes ( I can do some smoke testing tonight. |
What I meant to ask was: is it possible we might need another plugin for react-router? Ideally we can keep just one plugin, unless it becomes too messy e.g. for having to support multiple modes. |
No I don't think it will be any more modes we need to support in the future. The library mode for react router doesn't require a knip plugin and is already supported by how knip works today. |
|
|
||
| const isEnabled: IsPluginEnabled = ({ dependencies }) => hasDependency(dependencies, enablers); | ||
|
|
||
| const config: string[] = ['react-router.config.{js,ts}', ...vite.config]; |
There was a problem hiding this comment.
I added vite.config aswell because the react-router.config is optional.
There was a problem hiding this comment.
All config paths are loaded and if there's resolveConfig its executed with the default exported value of those config files. Also they're automatically added as entry files (so their imports are registered as dependencies like any other source file).
Since this plugin doesn't have resolveConfig I think 'react-router.config.{js,ts}' should be moved to the entry array (guess I've missed this before), and looks like there's no value in adding vite.config.
Just saying, the config array items do not influence whether the plugin is enabled or not.
There was a problem hiding this comment.
Any chance you could move config to entry and remove vite.config - I'd guess that gives the same results, right?
There was a problem hiding this comment.
Moving react-router.config.ts to entry and removing vite doesn't seem to enable the plugin. When console logging inside resolveEntryPaths nothing happends. Not sure whats happening.
There was a problem hiding this comment.
Ah sorry I looked over the fact that localConfig is an argument to resolveEntryPaths and thus a config item is required. Took the liberty to clean up and merge :)
|
I did some smoketesting on epic-stack and my personal projects and it seemed to work fine. Found some edge cases in the process which I fixed. One thing to mention is that react router generates types with the command |
Thanks a lot!
Good question, tbh I'm not sure. If it's recommended to generate those I guess not. Maybe if used in JavaScript-only context that would be annoying, and we could add an |
commit: |
webpro
force-pushed
the
react-router-7
branch
from
3ead7b9 to
4bb01b2
Compare
|
🚀 This pull request is included in v5.44.0. See Release 5.44.0 for release notes. Using Knip in a commercial project? Please consider becoming a sponsor. |
Hi, I was hoping to add support for React Router 7 framework mode 😄 I have tried my best to implement a plugin based on the docs. In React Router 7 the
routes.tsfile defines all the routes so I map over every route entry and their children, I think this is the correct way to do it. This plugin should also handle only js files aswell.If you use react router as library mode (only having
react-routeras dependency) the plugin should not be enabled. So I only enabled it when@react-router/devis present.I was a bit unsure what to include in
packages/knip/fixtures/plugins/react-router/**directory. I asumed that I only include mock files to be used when running the tests, so I omited the vite config for instance.