TanStack / router

🤖 Fully typesafe Router for React (and friends) w/ built-in caching, 1st class search-param APIs, client-side cache integration and isomorphic rendering.
https://tanstack.com/router
MIT License
7.27k stars 495 forks source link

Fixed root path to line up with router-generator #1259

Closed Kinqdos closed 2 months ago

Kinqdos commented 3 months ago

Fixes issue #1240

schiller-manuel commented 2 months ago

I am not sure process.cwd() is a good idea. What happens if vite is started from another directory? I think (but I probably not understand the problem fully) there must be another solution that does not rely on cwd.

Kinqdos commented 2 months ago

Yeah I see your point. Its the same behavior as the router cli. It searches for routes (and the config file) from the directory you started the cli / vite. This is already the behavior of the route generator. The problem is that the plugin wich handles the hmr updates always takes the vite root directory as "starting" point. (vite root is the directory with the index.html)

Another solution would be passing the vite root to the route generator instead of passing a relative directory. But therefore you also have to edit the router generator. As compromise you could replace the await getConfig(inlineConfig, ROOT) by await getConfig(inlineConfig, "."). This doesnt relies on cwd, but leads to the same result. In addtion you have to replace all join(ROOT, ...) with just resolve(...).

schiller-manuel commented 2 months ago

"edit the router generator " in which way?

Kinqdos commented 2 months ago

Because the generator function exported by the router-generator only takes the userConfig as argument. So you have to add the root directory as argument

schiller-manuel commented 2 months ago

i see no problem with this additional change, can you update the PR?

Kinqdos commented 2 months ago

Yes I can do! My concern is, that other packages wich also depend on the generatorfunction, will break with the changes to the arguments. P.S: The getConfig function from the router-generator also relies on cwd as fallback:

export async function getConfig(
  inlineConfig: Partial<Config> = {},
  configDirectory?: string,
): Promise<Config> {
  if (configDirectory === undefined) {
    configDirectory = process.cwd()
  }
...
Kinqdos commented 2 months ago

Any thoughts? Or should I start update the pr?

schiller-manuel commented 2 months ago

can't you add the root directory as optional argument, if it is not set, fall back to cwd?

Kinqdos commented 2 months ago

Just thinking if it is good to change the behavior here. The router-generator relies on cwd why can't the vite plugin? With passing the root to the generator we are introducing a different behavior between the vite plugin and the cli. The vite plugin is looking for config in the vite route directory, but if you switch over to using the cli nothing will work. The cli is looking for the config in the cwd and not in the vite root. So you have to move your config from the vite root to the cwd and update the paths in the config.

Is this a better solution than keeping the cwd solution in the vite-plugin?

schiller-manuel commented 2 months ago

ok, let's merge this, and wait for complaints :)

nx-cloud[bot] commented 2 months ago

☁️ Nx Cloud Report

CI is running/has finished running commands for commit dd1faa992bc3e1431b2e928d5c0baa24f58eb7ca. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 2 targets - [`nx affected --targets=test:format,test:lib,test:types,test:build,build --parallel=3`](https://cloud.nx.app/runs/0rbNbXYuBe?utm_source=pull-request&utm_medium=comment) - [`nx run-many --targets=test:lib,test:types,test:build,build --parallel=3`](https://cloud.nx.app/runs/rSdfsWVMZy?utm_source=pull-request&utm_medium=comment)

Sent with 💌 from NxCloud.