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

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

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

Kinqdos
Copy link
Contributor

@Kinqdos Kinqdos commented Mar 4, 2024

Fixes issue #1240

@schiller-manuel
Copy link
Contributor

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
Copy link
Contributor Author

Kinqdos commented Mar 15, 2024

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
Copy link
Contributor

"edit the router generator " in which way?

@Kinqdos
Copy link
Contributor Author

Kinqdos commented Mar 15, 2024

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
Copy link
Contributor

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

@Kinqdos
Copy link
Contributor Author

Kinqdos commented Mar 15, 2024

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
Copy link
Contributor Author

Kinqdos commented Mar 17, 2024

Any thoughts? Or should I start update the pr?

@schiller-manuel
Copy link
Contributor

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

@Kinqdos
Copy link
Contributor Author

Kinqdos commented Mar 18, 2024

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
Copy link
Contributor

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

Copy link

nx-cloud bot commented Mar 18, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit dd1faa9. 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

Sent with 💌 from NxCloud.

@schiller-manuel schiller-manuel merged commit c636b5c into TanStack:main Mar 18, 2024
2 checks passed
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.

None yet

2 participants