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

fix(react-router): implicit navigation for layout routes without a prefixed underscore id #1584

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions packages/react-router/src/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
functionalUpdate,
last,
pick,
removeLayoutSegments,
replaceEqualDeep,
} from './utils'
import { getRouteMatch } from './RouterProvider'
Expand Down Expand Up @@ -494,8 +493,7 @@ export class Router<
scores: Array<number>
}> = []

// eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion
const routes = Object.values(this.routesById) as Array<AnyRoute>
const routes: Array<AnyRoute> = Object.values(this.routesById)

routes.forEach((d, i) => {
if (d.isRoot || !d.path) {
Expand Down Expand Up @@ -937,17 +935,16 @@ export class Router<
fromMatches.find((e) => e.routeId === d.routeId),
)

const fromRouteByFromPath = stayingMatches?.find(
(d) => d.pathname === fromPath,
)
const fromRouteByFromPathRouteId =
this.routesById[
stayingMatches?.find((d) => d.pathname === fromPath)?.routeId
]

let pathname = dest.to
? this.resolvePathWithBase(fromPath, `${dest.to}`)
: this.resolvePathWithBase(
fromPath,
fromRouteByFromPath
? removeLayoutSegments(fromRouteByFromPath.routeId)
: fromPath,
fromRouteByFromPathRouteId?.to ?? fromPath,
)

const prevParams = { ...last(fromMatches)?.params }
Expand Down
14 changes: 0 additions & 14 deletions packages/react-router/src/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -339,20 +339,6 @@ export function createControlledPromise<T>(onResolve?: () => void) {
return controlledPromise
}

/**
* Removes all segments from a given path that start with an underscore ('_').
*
* @param {string} routePath - The path from which to remove segments. Defaults to '/'.
* @returns {string} The path with all underscore-prefixed segments removed.
* @example
* removeLayoutSegments('/workspace/_auth/foo') // '/workspace/foo'
*/
export function removeLayoutSegments(routePath: string): string {
const segments = routePath.split('/')
const newSegments = segments.filter((segment) => !segment.startsWith('_'))
return newSegments.join('/')
}

/**
* Taken from https://www.developerway.com/posts/implementing-advanced-use-previous-hook#part3
*/
Expand Down
69 changes: 58 additions & 11 deletions packages/react-router/tests/redirects.test.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
import { describe, expect, it } from 'vitest'

import {
type RouterHistory,
createMemoryHistory,
createRootRoute,
createRoute,
createRouter,
redirect,
type RouterHistory,
} from '../src'

function createTestRouter(initialHistory?: RouterHistory) {
Expand Down Expand Up @@ -40,16 +39,29 @@ function createTestRouter(initialHistory?: RouterHistory) {
path: '/$framework',
})

const userRoute = createRoute({
const uRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/u',
})
const userLayoutRoute = createRoute({
const uLayoutRoute = createRoute({
id: '_layout',
getParentRoute: () => userRoute,
getParentRoute: () => uRoute,
})
const usernameRoute = createRoute({
getParentRoute: () => userLayoutRoute,
const uUsernameRoute = createRoute({
getParentRoute: () => uLayoutRoute,
path: '$username',
})

const gRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/g',
})
const gLayoutRoute = createRoute({
id: 'layout',
getParentRoute: () => gRoute,
})
const gUsernameRoute = createRoute({
getParentRoute: () => gLayoutRoute,
path: '$username',
})

Expand All @@ -58,15 +70,15 @@ function createTestRouter(initialHistory?: RouterHistory) {
projectVersionRoute.addChildren([projectFrameRoute]),
]),
])
const userTree = userRoute.addChildren([
userLayoutRoute.addChildren([usernameRoute]),
])
const uTree = uRoute.addChildren([uLayoutRoute.addChildren([uUsernameRoute])])
const gTree = gRoute.addChildren([gLayoutRoute.addChildren([gUsernameRoute])])

const routeTree = rootRoute.addChildren([
indexRoute,
postsRoute.addChildren([postIdRoute]),
projectTree,
userTree,
uTree,
gTree,
])
const router = createRouter({ routeTree, history })

Expand Down Expand Up @@ -407,4 +419,39 @@ describe('router.navigate navigation using layout routes resolves correctly', as

expect(router.state.location.pathname).toBe('/u/tkdodo')
})

it('should resolve "/g/tanner" in "/g/layout/$username" to "/g/tkdodo"', async () => {
const { router } = createTestRouter(
createMemoryHistory({ initialEntries: ['/g/tanner'] }),
)

await router.load()

expect(router.state.location.pathname).toBe('/g/tanner')

await router.navigate({
to: '/g/$username',
params: { username: 'tkdodo' },
})
await router.invalidate()

expect(router.state.location.pathname).toBe('/g/tkdodo')
})

it('should resolve "/g/tanner" in "/g/layout/$username" to "/g/tkdodo" w/o "to" path being provided', async () => {
const { router } = createTestRouter(
createMemoryHistory({ initialEntries: ['/g/tanner'] }),
)

await router.load()

expect(router.state.location.pathname).toBe('/g/tanner')

await router.navigate({
params: { username: 'tkdodo' },
})
await router.invalidate()

expect(router.state.location.pathname).toBe('/g/tkdodo')
})
})
51 changes: 0 additions & 51 deletions packages/react-router/tests/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { describe, expect, it } from 'vitest'
import {
exactPathTest,
isPlainArray,
removeLayoutSegments,
removeTrailingSlash,
replaceEqualDeep,
} from '../src/utils'
Expand Down Expand Up @@ -375,53 +374,3 @@ describe('exactPathTest', () => {
expect(result).toBe(true)
})
})

describe('removeLayoutSegments', () => {
it('should remove the "_layout" segment from "/_layout/" and return "/"', () => {
const path = '/_layout/'
const result = removeLayoutSegments(path)
expect(result).toBe('/')
})

it('should remove the "_layout" segment from "/_layout/blog" and return "/blog"', () => {
const path = '/_layout/blog'
const result = removeLayoutSegments(path)
expect(result).toBe('/blog')
})

it('should remove the "_layout1" and "_layout2" segments from "/_layout1/" and return "/"', () => {
const path = '/_layout1/_layout2/'
const result = removeLayoutSegments(path)
expect(result).toBe('/')
})

it('should remove the "_layout1" and "_layout2" segments from "/_layout1/blog" and return "/blog"', () => {
const path = '/_layout1/_layout2/blog'
const result = removeLayoutSegments(path)
expect(result).toBe('/blog')
})

it('should remove the "_layout" segment from "/posts/_layout/" and return "/posts/"', () => {
const path = '/posts/_layout/1'
const result = removeLayoutSegments(path)
expect(result).toBe('/posts/1')
})

it('should remove the "_layout" segment from "/posts/_layout/1" and return "/posts/1"', () => {
const path = '/posts/_layout/1'
const result = removeLayoutSegments(path)
expect(result).toBe('/posts/1')
})

it('should remove the "_layout" segment from "/posts/_layout/" and return "/posts"', () => {
const path = '/posts/_layout/'
const result = removeLayoutSegments(path)
expect(result).toBe('/posts/')
})

it('should remove the "_layout" segment from "/posts/_layout/blog" and return "/posts/blog"', () => {
const path = '/posts/_layout/blog'
const result = removeLayoutSegments(path)
expect(result).toBe('/posts/blog')
})
})
Loading