-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Fix root directory resolution logic #13472
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
Conversation
🦋 Changeset detectedLatest commit: 1d760f6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
root: string, | ||
flags: { | ||
watch?: boolean; | ||
config?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pcattori Just calling out that since the --config
flag impacts the project root directory (and hence where routes live etc.), I think it makes sense to support this in all CLI commands for consistency, even those that don't use the Vite config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Side note, but I think this raises something I might have missed when we introduced react-router.config.ts
, which is that the --config
flag probably should have been updated to point to our config, and any custom Vite config path should have been configured there, or via a separate --vite-config
flag.
This is part of my rationale for this change. If our --config
flag pointed to react-router.config.ts
, I would want to support it here too.
if (root) { | ||
return path.resolve(root); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that if a root directory was explicitly provided (e.g. react-router build my/app/dir
) this will still take precedence. We only fall back to inferring the root when this isn't provided, so anyone relying on this should maintain existing behaviour.
🤖 Hello there, We just published version Thanks! |
Fixes #12390
This PR fixes a few issues that were raised by the issue linked above:
react-router dev --config apps/my-app/vite.config.ts
), the React Router project directory would still becwd
. This meant that you'd get an error about a missing root route even if it's present in the directory containing the Vite config. To fix this, if a custom Vite config path is provided, we now default the React Router project root to be the directory containing the custom Vite config.resolveRootDirectory
util which is used consistently across all commands. This gives us a central place to implement the fix mentioned above. This issue wasn't explicitly raised, but was highlighted when fixing the above issue.package.json
but a parent directory has one, an error is thrown because we try to read the non-existentpackage.json
from root. To fix this, we now also walk parent directories to findpackage.json
. If none of the parent directories contain apackage.json
, you'll still see the same error as before.Note that I've opted not to test this logic specifically. I've manually tested in one of our playgrounds, moving all app code into a subdirectory and ensuring I can do the following:
react-router dev apps/my-app
.react-router dev --config apps/my-app/vite.config.ts
I've also tried to ensure this doesn't break any usages that inadvertently work today (e.g. having config files in a
config
dir and theapp
directory in the root), but this just showed me that too much of our code assumes everything is in the root directory. If the Vite config is in aconfig
dir, areact-router.config.ts
file next to it doesn't get picked up. In this scenario, we also generate a.react-router
dir in theconfig
dir. This makes me much more confident that it's safe to infer the root directory from the Vite config location since it currently doesn't seem to work otherwise.