|
| 1 | +# Plan: Remove "Agnostic" Layer from RouteObject/RouteMatch Types |
| 2 | + |
| 3 | +**Goal**: Consolidate the framework-agnostic and React-specific type layers into a single React-aware layer without breaking the public API. |
| 4 | + |
| 5 | +## Background |
| 6 | + |
| 7 | +React Router historically maintained a framework-agnostic layer to support multiple UI frameworks. Since we no longer support other frameworks, we can simplify our type hierarchy by removing this abstraction. |
| 8 | + |
| 9 | +### Current Structure |
| 10 | + |
| 11 | +**Agnostic Layer** (`lib/router/utils.ts`): |
| 12 | + |
| 13 | +- `AgnosticBaseRouteObject` - Base route properties (loader, action, etc.) |
| 14 | +- `AgnosticIndexRouteObject` - Index route without children |
| 15 | +- `AgnosticNonIndexRouteObject` - Route with optional children |
| 16 | +- `AgnosticRouteObject` - Union of index/non-index routes |
| 17 | +- `AgnosticDataIndexRouteObject` - Index route with required `id` |
| 18 | +- `AgnosticDataNonIndexRouteObject` - Non-index route with required `id` |
| 19 | +- `AgnosticDataRouteObject` - Data route with required `id` |
| 20 | +- `AgnosticRouteMatch` - Match result |
| 21 | +- `AgnosticDataRouteMatch` - Data route match result |
| 22 | + |
| 23 | +**React Layer** (`lib/context.ts`): |
| 24 | + |
| 25 | +- `IndexRouteObject` - Extends agnostic type + React fields (`element`, `Component`, `errorElement`, `ErrorBoundary`, `hydrateFallbackElement`, `HydrateFallback`) |
| 26 | +- `NonIndexRouteObject` - Extends agnostic type + React fields |
| 27 | +- `RouteObject` - Union of index/non-index |
| 28 | +- `DataRouteObject` - Route with required `id` |
| 29 | +- `RouteMatch` - Extends `AgnosticRouteMatch` |
| 30 | +- `DataRouteMatch` - Extends `AgnosticRouteMatch` |
| 31 | + |
| 32 | +### Problem |
| 33 | + |
| 34 | +The two-layer structure adds complexity: |
| 35 | + |
| 36 | +1. Duplicate type definitions with subtle differences |
| 37 | +2. Requires understanding inheritance hierarchy |
| 38 | +3. The "agnostic" layer no longer serves a purpose |
| 39 | +4. React types extend agnostic types just to add React-specific fields |
| 40 | + |
| 41 | +## Proposed Solution |
| 42 | + |
| 43 | +**Strategy**: Consolidate types by renaming the "agnostic" types to be the primary React-aware types, eliminating the prefix entirely. |
| 44 | + |
| 45 | +### Approach: Rename and Merge (Alternative 1) |
| 46 | + |
| 47 | +Since `Agnostic*` types are not exported from the public API, we can safely rename them to become the primary `RouteObject`, `RouteMatch`, etc. types. The React layer in `context.ts` will re-export these types, maintaining the public API contract. |
| 48 | + |
| 49 | +#### Step 1: Update `lib/router/utils.ts` |
| 50 | + |
| 51 | +Rename existing `Agnostic*` types and add React-specific fields: |
| 52 | + |
| 53 | +```typescript |
| 54 | +/** |
| 55 | + * Base RouteObject with common props shared by all types of routes |
| 56 | + */ |
| 57 | +type BaseRouteObject = { |
| 58 | + caseSensitive?: boolean; |
| 59 | + path?: string; |
| 60 | + id?: string; |
| 61 | + middleware?: MiddlewareFunction[]; |
| 62 | + loader?: LoaderFunction | boolean; |
| 63 | + action?: ActionFunction | boolean; |
| 64 | + hasErrorBoundary?: boolean; |
| 65 | + shouldRevalidate?: ShouldRevalidateFunction; |
| 66 | + handle?: any; |
| 67 | + lazy?: LazyRouteDefinition<RouteObject>; |
| 68 | + // React-specific fields (merged from context.ts) |
| 69 | + element?: React.ReactNode | null; |
| 70 | + hydrateFallbackElement?: React.ReactNode | null; |
| 71 | + errorElement?: React.ReactNode | null; |
| 72 | + Component?: React.ComponentType | null; |
| 73 | + HydrateFallback?: React.ComponentType | null; |
| 74 | + ErrorBoundary?: React.ComponentType | null; |
| 75 | +}; |
| 76 | + |
| 77 | +/** |
| 78 | + * Index routes must not have children |
| 79 | + */ |
| 80 | +export type IndexRouteObject = BaseRouteObject & { |
| 81 | + children?: undefined; |
| 82 | + index: true; |
| 83 | +}; |
| 84 | + |
| 85 | +/** |
| 86 | + * Non-index routes may have children, but cannot have index |
| 87 | + */ |
| 88 | +export type NonIndexRouteObject = BaseRouteObject & { |
| 89 | + children?: RouteObject[]; |
| 90 | + index?: false; |
| 91 | +}; |
| 92 | + |
| 93 | +/** |
| 94 | + * A route object represents a logical route, with (optionally) its child |
| 95 | + * routes organized in a tree-like structure. |
| 96 | + */ |
| 97 | +export type RouteObject = IndexRouteObject | NonIndexRouteObject; |
| 98 | + |
| 99 | +export type DataIndexRouteObject = IndexRouteObject & { |
| 100 | + id: string; |
| 101 | +}; |
| 102 | + |
| 103 | +export type DataNonIndexRouteObject = NonIndexRouteObject & { |
| 104 | + children?: DataRouteObject[]; |
| 105 | + id: string; |
| 106 | +}; |
| 107 | + |
| 108 | +/** |
| 109 | + * A data route object, which is just a RouteObject with a required unique ID |
| 110 | + */ |
| 111 | +export type DataRouteObject = DataIndexRouteObject | DataNonIndexRouteObject; |
| 112 | + |
| 113 | +/** |
| 114 | + * A RouteMatch contains info about how a route matched a URL. |
| 115 | + */ |
| 116 | +export interface RouteMatch< |
| 117 | + ParamKey extends string = string, |
| 118 | + RouteObjectType extends RouteObject = RouteObject, |
| 119 | +> { |
| 120 | + /** |
| 121 | + * The names and values of dynamic parameters in the URL. |
| 122 | + */ |
| 123 | + params: Params<ParamKey>; |
| 124 | + /** |
| 125 | + * The portion of the URL pathname that was matched. |
| 126 | + */ |
| 127 | + pathname: string; |
| 128 | + /** |
| 129 | + * The portion of the URL pathname that was matched before child routes. |
| 130 | + */ |
| 131 | + pathnameBase: string; |
| 132 | + /** |
| 133 | + * The route object that was used to match. |
| 134 | + */ |
| 135 | + route: RouteObjectType; |
| 136 | +} |
| 137 | + |
| 138 | +export interface DataRouteMatch extends RouteMatch<string, DataRouteObject> {} |
| 139 | +``` |
| 140 | + |
| 141 | +Key changes: |
| 142 | + |
| 143 | +- `AgnosticBaseRouteObject` → `BaseRouteObject` (add React fields) |
| 144 | +- `AgnosticIndexRouteObject` → `IndexRouteObject` |
| 145 | +- `AgnosticNonIndexRouteObject` → `NonIndexRouteObject` |
| 146 | +- `AgnosticRouteObject` → `RouteObject` |
| 147 | +- `AgnosticDataIndexRouteObject` → `DataIndexRouteObject` |
| 148 | +- `AgnosticDataNonIndexRouteObject` → `DataNonIndexRouteObject` |
| 149 | +- `AgnosticDataRouteObject` → `DataRouteObject` |
| 150 | +- `AgnosticRouteMatch` → `RouteMatch` |
| 151 | +- `AgnosticDataRouteMatch` → `DataRouteMatch` |
| 152 | + |
| 153 | +#### Step 2: Update `lib/context.ts` |
| 154 | + |
| 155 | +Convert to simple re-exports since the types now exist in `utils.ts`: |
| 156 | + |
| 157 | +```typescript |
| 158 | +// Re-export route types from utils (they're now React-aware) |
| 159 | +export type { |
| 160 | + IndexRouteObject, |
| 161 | + NonIndexRouteObject, |
| 162 | + RouteObject, |
| 163 | + DataRouteObject, |
| 164 | + RouteMatch, |
| 165 | + DataRouteMatch, |
| 166 | +} from "./router/utils"; |
| 167 | + |
| 168 | +// PatchRoutesOnNavigationFunction types can now be simplified |
| 169 | +export type PatchRoutesOnNavigationFunctionArgs = |
| 170 | + AgnosticPatchRoutesOnNavigationFunctionArgs<RouteObject, RouteMatch>; |
| 171 | + |
| 172 | +export type PatchRoutesOnNavigationFunction = |
| 173 | + AgnosticPatchRoutesOnNavigationFunction<RouteObject, RouteMatch>; |
| 174 | +``` |
| 175 | + |
| 176 | +Remove duplicate type definitions and re-export from `utils.ts`: |
| 177 | + |
| 178 | +**Before:** |
| 179 | +```typescriptimport or reference `Agnostic\*` types: |
| 180 | + |
| 181 | +**Files to update:** |
| 182 | + |
| 183 | +1. `lib/router/router.ts` - Update all `AgnosticDataRouteObject` → `DataRouteObject`, `AgnosticDataRouteMatch` → `DataRouteMatch`, `AgnosticRouteObject` → `RouteObject` |
| 184 | +2. `lib/router/instrumentation.ts` - Update route type references |
| 185 | +3. `lib/router/utils.ts` - Update internal function signatures, type parameters, and helper functions |
| 186 | +4. `lib/dom/ssr/links.ts` - Update `AgnosticDataRouteMatch` → `DataRouteMatch` |
| 187 | +5. `lib/rsc/server.rsc.ts` - Update `AgnosticDataRouteMatch` → `DataRouteMatch` |
| 188 | +6. `lib/context.ts` - Update `AgnosticPatchRoutesOnNavigationFunctionArgs` references (or rename those too) |
| 189 | +7. `__tests__/**/*.ts` - Update test type references (~40 files) |
| 190 | + |
| 191 | +**Note**: Some files may still reference `Agnostic*` types that are part of function/generic names (like `AgnosticPatchRoutesOnNavigationFunction`). These can be renamed in a follow-up or kept as-is if the name makes sense in context. |
| 192 | + |
| 193 | +```` |
| 194 | +
|
| 195 | +**After:** |
| 196 | +```typescript |
| 197 | +// Re-export route types from utils (they're now React-aware) |
| 198 | +export type { |
| 199 | + IndexRouteObject, |
| 200 | + NonIndexRouteObject, |
| 201 | + RouteObject, |
| 202 | + DataRouteObject, |
| 203 | + RouteMatch, |
| 204 | + DataRouteMatch, |
| 205 | +} from "./router/utils"; |
| 206 | +
|
| 207 | +// PatchRoutesOnNavigationFunction types remain the same |
| 208 | +export type PatchRoutesOnNavigationFunctionArgs = |
| 209 | + AgnosticPatchRoutesOnNavigationFunctionArgs<RouteObject, RouteMatch>; |
| 210 | +
|
| 211 | +export type PatchRoutesOnNavigationFunction = |
| 212 | + AgnosticPatchRoutesOnNavigationFunction<RouteObject, RouteMatch>; |
| 213 | +```` |
| 214 | + |
| 215 | +This eliminates ~70 lines of duplicate type definitions.ction matchRoutes< |
| 216 | +RouteObjectType extends AgnosticRouteObject = AgnosticRouteObject |
| 217 | + |
| 218 | +> (...) |
| 219 | +
|
| 220 | +// After |
| 221 | +function matchRoutes< |
| 222 | +RouteObjectType extends RouteObject = RouteObject |
| 223 | + |
| 224 | +> (...) |
| 225 | +
|
| 226 | +```` |
| 227 | +
|
| 228 | +#### Step 5: Update Helper Functions |
| 229 | +
|
| 230 | +Update type guards and helper functions: |
| 231 | +
|
| 232 | +```typescript |
| 233 | +// Before |
| 234 | +function isIndexRoute( |
| 235 | + route: AgnosticRouteObject, |
| 236 | +): route is AgnosticIndexRouteObject |
| 237 | +
|
| 238 | +// After |
| 239 | +function isIndexRoute( |
| 240 | + route: RouteObject, |
| 241 | +): route is IndexRouteObject |
| 242 | +```` |
| 243 | + |
| 244 | +**No migration needed!** |
| 245 | + |
| 246 | +Since `Agnostic*` types are not part of the public API (not exported from `index.ts`), this is purely an internal refactoring. External consumers only use `RouteObject`, `DataRouteObject`, `RouteMatch`, and `DataRouteMatch` which remain unchanged in the public API.`RouteMatch`, `DataRouteMatch`) still exported |
| 247 | + |
| 248 | +**Internal API**: ⚠️ Requires updates |
| 249 | + |
| 250 | +- Functions using `Agnostic*` types need updating |
| 251 | +- Tests using `Agnostic*` types need updating |
| 252 | +- Backward compatibility aliases prevent hard breaks |
| 253 | + |
| 254 | +**Type Safety**: ✅ Maintained |
| 255 | + |
| 256 | +- All types remain strongly typed |
| 257 | +- React-specific fields properly typed throughout |
| 258 | +- No loss of type information |
| 259 | + |
| 260 | +### Migration Path for Consumers |
| 261 | + |
| 262 | +For any external code using the `Agnostic*` types (unlikely since they're not exported): |
| 263 | + |
| 264 | +1. **Short term**: Backward compatibility aliases work transparently |
| 265 | +2. **Medium term**: Types marked `@internal` and `@deprecated` |
| 266 | +3. **Long term**: Remove aliases in next major version |
| 267 | + |
| 268 | +### Testing Strategy |
| 269 | + |
| 270 | +1. **Type checks**: Ensure `pnpm typecheck` passes |
| 271 | +2. **Unit tests**: Ensure `pnpm test` passes |
| 272 | +3. **Integration tests**: Ensure `pnpm test:integration --project chromium` passes |
| 273 | +4. **Build**: Ensure `pnpm build` succeeds |
| 274 | +5. **Docs generation**: Ensure `pnpm docs` works correctly |
| 275 | + |
| 276 | +### Implementation Order |
| 277 | + |
| 278 | +### Implementation Order |
| 279 | + |
| 280 | +1. ✅ Create this plan document |
| 281 | +2. ✅ Update `lib/router/utils.ts` - Rename `Agnostic*` types to remove prefix, add React fields to base types |
| 282 | +3. ✅ Update `lib/context.ts` - Remove duplicate definitions, convert to re-exports |
| 283 | +4. ✅ Update `lib/router/router.ts` - Replace all `Agnostic*` type references |
| 284 | +5. ✅ Update `lib/router/instrumentation.ts` - Replace `Agnostic*` type references |
| 285 | +6. ✅ Update `lib/dom/ssr/links.ts` - Replace `AgnosticDataRouteMatch` references |
| 286 | +7. ✅ Update `lib/rsc/server.rsc.ts` - Replace `AgnosticDataRouteMatch` references |
| 287 | +8. ✅ Update test files - Replace `Agnostic*` type references (~40 files) - No test files needed updating |
| 288 | +9. ✅ Run full test suite (`pnpm typecheck && pnpm test && pnpm test:integration --project chromium`) - All passing |
| 289 | +10. ✅ Update JSDoc comments if any reference "agnostic" or "framework-agnostic" |
| 290 | +11. ✅ Create changeset |
| 291 | + |
| 292 | +### Risks & Mitigations |
| 293 | + |
| 294 | +Accidentally changing public API behavior |
| 295 | + |
| 296 | +- **Mitigation**: Public exports in `index.ts` remain unchanged, only importing from same location (`./lib/context`) |
| 297 | + |
| 298 | +**Risk**: Type inference changes in complex generic scenarios |
| 299 | + |
| 300 | +- **Mitigation**: All type fields remain identical, just location changes. Run full typecheck suite. |
| 301 | + |
| 302 | +**Risk**: Complex merge conflicts if done across multiple PRs |
| 303 | + |
| 304 | +- **Mitigation**: Complete in single atomic PR |
| 305 | + |
| 306 | +**Risk**: Missing some `Agnostic*` type references in large codebase |
| 307 | + |
| 308 | +- **Mitigation**: TypeScript compiler will catch all references; can also use global find/replace to identify all usages first |
| 309 | +- **Mitigation**: Thorough testing at each step, update tests incrementally |
| 310 | + |
| 311 | +## Alternative Approaches Considered |
| 312 | + |
| 313 | +### Alternative 1: Rename to Remove "Agnostic" Prefix |
| 314 | + |
| 315 | +Move all types f2: In-Place Consolidation with Aliases |
| 316 | + |
| 317 | +Keep types in their current files but merge the layers by making the "agnostic" types React-aware and converting the React layer to aliases: |
| 318 | + |
| 319 | +- `AgnosticBaseRouteObject` stays in `utils.ts` but gains React fields |
| 320 | +- React types in `context.ts` become aliases: `export type RouteObject = AgnosticRouteObject` |
| 321 | +- Add `@deprecated` backward compatibility aliases |
| 322 | + |
| 323 | +**Pros**: Gradual migration path with deprecated aliases |
| 324 | +**Cons**: Keeps confusing "Agnostic" naming, unnecessary since types aren't exported |
| 325 | + |
| 326 | +### Alternative 3but make React types standalone (not extending): |
| 327 | + |
| 328 | +- Both define all fields independently |
| 329 | +- No inheritance relationship |
| 330 | + |
| 331 | +**Pros**: Minimal code changes |
| 332 | +**Cons**: Doesn't reduce duplication, doesn't achieve goal |
| 333 | + |
| 334 | +### Alternative 3: Gradual Deprecation Path |
| 335 | + |
| 336 | +4: Gradual Deprecation Path |
| 337 | + |
| 338 | +Add new types, deprecate old ones, remove later: |
| 339 | + |
| 340 | +- Create `RouteObjectV2` with merged types |
| 341 | +- Deprecate `AgnosticRouteObject` and React `RouteObject` |
| 342 | +- Remove in next major |
| 343 | + |
| 344 | +**Pros**: Maximum safety, clear migration path |
| 345 | +**Cons**: Temporary duplication, longer timeline, unnecessary complexity |
| 346 | + |
| 347 | +## Decision |
| 348 | + |
| 349 | +**Selected Approach**: Rename and Merge (Alternative 1, now main proposal) |
| 350 | + |
| 351 | +**Rationale**: |
| 352 | + |
| 353 | +1. ✅ Achieves goal of removing agnostic layer completely |
| 354 | +2. ✅ **Non-breaking** - `Agnostic*` types are not in public API |
| 355 | +3. ✅ Cleaner - removes confusing "Agnostic" prefix entirely |
| 356 | +4. ✅ Simpler - no deprecated aliases needed since types aren't exported |
| 357 | +5. ✅ Single atomic change is easier to review and test |
| 358 | +6. ✅ Types live in one logical place (`utils.ts`) |
| 359 | +7. ✅ Reduces ~70 lines of duplicate type definitions in `context.ts` |
| 360 | + |
| 361 | +## Follow-up Work |
| 362 | + |
| 363 | +After this refactoring: |
| 364 | + |
| 365 | +1. Consider if `RouteManifest<R = DataRouteObject>` generic parameter is still needed |
| 366 | +2. Evaluate if other "agnostic" patterns exist elsewhere |
| 367 | +3. Update internal documentation about type architecture |
| 368 | +4. Consider removing `@internal` aliases in v8 |
0 commit comments