-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
fix: generate relative route IDs when using relative() helper #14156
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
base: dev
Are you sure you want to change the base?
Conversation
|
Hi @joseph0926, Welcome, and thank you for contributing to React Router! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at [email protected]. Thanks! - The Remix team |
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
5211322
to
345882a
Compare
To ensure this PR reliably fixes the issue and is production-safe, I've added 35 test cases covering issue reproduction, Windows paths, special characters, environment independence, function overloads, and integration scenarios |
@@ -318,6 +318,12 @@ export { route, index, layout, prefix }; | |||
* splitting route config into multiple files within different directories. | |||
*/ | |||
export function relative(directory: string): typeof helpers { | |||
const generateRelativeId = (file: string): 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.
Helper function to generate consistent relative path IDs. This ensures route IDs are always relative to the project root, not absolute file system paths. Uses the existing createRouteId
utility for consistency with the rest of the codebase.
const absolutePath = Path.resolve(directory, file); | ||
const relativeId = generateRelativeId(file); | ||
|
||
if (!rest || (rest as any).length === 0) { |
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.
when no additional arguments are provided, we simply add the generated relative ID to ensure it's not created as an absolute path later in configRoutesToRouteManifest
.
} | ||
|
||
const options = first as CreateRouteOptions; | ||
const modifiedOptions = options.id |
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.
Preserve explicitly provided IDs while adding relative IDs for routes without them. This maintains backward compatibility
/** | ||
* Helper function for creating a route config entry for an index route, for | ||
* use within `routes.ts`. Note that this helper has been scoped, meaning | ||
* that file path will be resolved relative to the directory provided to the | ||
* `relative` call that created this helper. | ||
*/ | ||
index: (file, ...rest) => { | ||
return index(Path.resolve(directory, file), ...(rest as any)); | ||
const absolutePath = Path.resolve(directory, file); |
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.
Simplified version of the route logic since index() has fewer overloads. (generate relative ID, preserve explicit IDs if provided.)
Hi @markdalgleish @pcattori , When you have a chance, could you take a look at this PR? Happy to address any feedback or questions about the implementation. Thanks! |
Fixes #14125
Note: I'm not a native English speaker and used translation tools for this PR.
Problem
When using the
relative()
helper, route IDs were generated using absolute file paths instead of relative paths.Solution
Modified
relative()
helper to pre-generate route IDs based on relative paths. This preventsconfigRoutesToRouteManifest
from creating absolute path IDs later.Implementation Process
relative()
functionThe key insight:
configRoutesToRouteManifest
always generates an ID if not provided. By pre-generating relative IDs inrelative()
, we prevent absolute path IDs from being created.Technical Details
TypeScript Challenge
The
rest
parameter was inferred as tuple type1 | 2
due to function overloads. Had to use type casting for length checks:Why IDs are added
configRoutesToRouteManifest
callscreateRouteId(absolutePath)
→ absolute IDconfigRoutesToRouteManifest
uses it → relative IDChanges
packages/react-router-dev/config/routes.ts
: Modifiedrelative()
to generate IDspackages/react-router-dev/__tests__/relative-test.ts
: Added comprehensive testspackages/react-router-dev/__tests__/route-config-test.ts
: Updated snapshots (IDs now present)Test Results
pnpm test packages/react-router-dev/ ✓ All 155 tests passing ✓ Snapshots updated to reflect ID generation
The snapshot changes show routes created with
relative()
now have IDs:Breaking Changes
None. This fix only affects automatic ID generation when using
relative()
helper.