Skip to content
Open
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
24 changes: 15 additions & 9 deletions docs/router/framework/react/api/router/useBlockerHook.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ interface ShouldBlockFnLocation<...> {
}

type ShouldBlockFnArgs = {
current: ShouldBlockFnLocation
next: ShouldBlockFnLocation
current: ShouldBlockFnLocation | undefined
next: ShouldBlockFnLocation | undefined
action: HistoryAction
}
```
Expand All @@ -53,6 +53,12 @@ type ShouldBlockFnArgs = {
- Type: `boolean`
- Specify if the resolver returned by the hook should be used or whether your `shouldBlockFn` function itself resolves the blocking

### `options.throwOnUnknownRoute` option

- Optional - defaults to `true`
- Type: `boolean`
- Specify if the hook should throw an exception when either the current or next locations is unkown

Comment on lines +56 to +61
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix typo and grammar in the throwOnUnknownRoute option description.

“locations is unkown” → “location cannot be resolved” (and singular agreement). This is user-facing; fix it.

-### `options.throwOnUnknownRoute` option
-
-- Optional - defaults to `true`
-- Type: `boolean`
-- Specify if the hook should throw an exception when either the current or next locations is unkown
+### `options.throwOnUnknownRoute` option
+
+- Optional - defaults to `true`
+- Type: `boolean`
+- Specify whether the hook should throw when either the current or next location cannot be resolved.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
### `options.throwOnUnknownRoute` option
- Optional - defaults to `true`
- Type: `boolean`
- Specify if the hook should throw an exception when either the current or next locations is unkown
### `options.throwOnUnknownRoute` option
- Optional - defaults to `true`
- Type: `boolean`
- Specify whether the hook should throw when either the current or next location cannot be resolved.
🧰 Tools
🪛 LanguageTool

[grammar] ~58-~58: There might be a mistake here.
Context: ...Routeoption - Optional - defaults totrue- Type:boolean` - Specify if the hook sh...

(QB_NEW_EN)


[grammar] ~59-~59: There might be a mistake here.
Context: ...- Optional - defaults to true - Type: boolean - Specify if the hook should throw an exce...

(QB_NEW_EN)

🤖 Prompt for AI Agents
In docs/router/framework/react/api/router/useBlockerHook.md around lines 56 to
61, the description for the `options.throwOnUnknownRoute` option contains a typo
and grammatical error; update the bullet text so it reads: "Specify if the hook
should throw an exception when either the current or next location cannot be
resolved" (use singular "location" and the phrase "cannot be resolved" instead
of "is unkown").

### `options.blockerFn` option (⚠️ deprecated)

- Optional
Expand Down Expand Up @@ -120,7 +126,7 @@ function MyComponent() {
{/* ... */}
{status === 'blocked' && (
<div>
<p>You are navigating to {next.pathname}</p>
<p>You are navigating to {next?.pathname ?? 'Unknown Path'}</p>
<p>Are you sure you want to leave?</p>
<button onClick={proceed}>Yes</button>
<button onClick={reset}>No</button>
Expand All @@ -138,7 +144,7 @@ import { useBlocker } from '@tanstack/react-router'
function MyComponent() {
const { proceed, reset, status } = useBlocker({
shouldBlockFn: ({ next }) => {
return !next.pathname.includes('step/')
return next && !next.pathname.includes('step/')
},
Comment on lines 146 to 148
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Return type bug: expression can return undefined, not boolean.

return next && ... yields undefined when next is undefined, violating the documented contract that shouldBlockFn returns a boolean. Use a ternary or double negation.

-      return next && !next.pathname.includes('step/')
+      // If `next` is unknown, do not block
+      return next ? !next.pathname.includes('step/') : false
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
shouldBlockFn: ({ next }) => {
return !next.pathname.includes('step/')
return next && !next.pathname.includes('step/')
},
shouldBlockFn: ({ next }) => {
// If `next` is unknown, do not block
return next ? !next.pathname.includes('step/') : false
},
🤖 Prompt for AI Agents
In docs/router/framework/react/api/router/useBlockerHook.md around lines 146 to
148, the shouldBlockFn uses "return next && !next.pathname.includes('step/')"
which can return undefined when next is undefined; change it to return an
explicit boolean such as "next ? !next.pathname.includes('step/')" or use
double-negation "!!next && !next.pathname.includes('step/')" so the function
always returns a boolean.

withResolver: true,
})
Expand Down Expand Up @@ -170,7 +176,7 @@ function MyComponent() {

useBlocker({
shouldBlockFn: ({ next }) => {
if (next.pathname.includes('step/')) {
if (next?.pathname.includes('step/')) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Optional chaining is incomplete; potential runtime error.

next?.pathname.includes(...) will still call .includes on undefined when pathname is missing. Chain ?. on pathname as well.

-      if (next?.pathname.includes('step/')) {
+      if (next?.pathname?.includes('step/')) {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (next?.pathname.includes('step/')) {
if (next?.pathname?.includes('step/')) {
🤖 Prompt for AI Agents
In docs/router/framework/react/api/router/useBlockerHook.md around line 179, the
expression next?.pathname.includes('step/') can throw if pathname is undefined;
change it to use optional chaining on pathname as well
(next?.pathname?.includes('step/')) so .includes is only called when pathname
exists.

return false
}

Expand All @@ -195,10 +201,10 @@ function MyComponent() {
const { proceed, reset, status } = useBlocker({
shouldBlockFn: ({ current, next }) => {
if (
current.routeId === '/editor-1' &&
next.fullPath === '/foo/$id' &&
next.params.id === '123' &&
next.search.hello === 'world'
current?.routeId === '/editor-1' &&
next?.fullPath === '/foo/$id' &&
next?.params.id === '123' &&
next?.search.hello === 'world'
) {
return true
}
Expand Down
25 changes: 19 additions & 6 deletions packages/react-router/src/useBlocker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ type MakeShouldBlockFnLocationUnion<
type BlockerResolver<TRouter extends AnyRouter = RegisteredRouter> =
| {
status: 'blocked'
current: MakeShouldBlockFnLocationUnion<TRouter>
next: MakeShouldBlockFnLocationUnion<TRouter>
current: MakeShouldBlockFnLocationUnion<TRouter> | undefined
next: MakeShouldBlockFnLocationUnion<TRouter> | undefined
action: HistoryAction
proceed: () => void
reset: () => void
Expand All @@ -57,8 +57,8 @@ type BlockerResolver<TRouter extends AnyRouter = RegisteredRouter> =
}

type ShouldBlockFnArgs<TRouter extends AnyRouter = RegisteredRouter> = {
current: MakeShouldBlockFnLocationUnion<TRouter>
next: MakeShouldBlockFnLocationUnion<TRouter>
current: MakeShouldBlockFnLocationUnion<TRouter> | undefined
next: MakeShouldBlockFnLocationUnion<TRouter> | undefined
action: HistoryAction
}

Expand All @@ -73,6 +73,7 @@ export type UseBlockerOpts<
enableBeforeUnload?: boolean | (() => boolean)
disabled?: boolean
withResolver?: TWithResolver
throwOnUnknownRoute?: boolean
}

type LegacyBlockerFn = () => Promise<any> | any
Expand Down Expand Up @@ -157,6 +158,7 @@ export function useBlocker(
enableBeforeUnload = true,
disabled = false,
withResolver = false,
throwOnUnknownRoute = true,
} = _resolveBlockerOpts(opts, condition)

const router = useRouter()
Expand All @@ -175,14 +177,14 @@ export function useBlocker(
const blockerFnComposed = async (blockerFnArgs: BlockerFnArgs) => {
function getLocation(
location: HistoryLocation,
): AnyShouldBlockFnLocation {
): AnyShouldBlockFnLocation | undefined {
const parsedLocation = router.parseLocation(location)
const matchedRoutes = router.getMatchedRoutes(
parsedLocation.pathname,
undefined,
)
if (matchedRoutes.foundRoute === undefined) {
throw new Error(`No route found for location ${location.href}`)
return undefined
}
return {
routeId: matchedRoutes.foundRoute.id,
Expand All @@ -194,7 +196,17 @@ export function useBlocker(
}

const current = getLocation(blockerFnArgs.currentLocation)
if (!current && throwOnUnknownRoute) {
throw new Error(
`No route found for location ${blockerFnArgs.currentLocation.href}`,
)
}
const next = getLocation(blockerFnArgs.nextLocation)
if (!next && throwOnUnknownRoute) {
throw new Error(
`No route found for location ${blockerFnArgs.nextLocation.href}`,
)
}

const shouldBlock = await shouldBlockFn({
action: blockerFnArgs.action,
Expand Down Expand Up @@ -243,6 +255,7 @@ export function useBlocker(
withResolver,
history,
router,
throwOnUnknownRoute
])

return resolver
Expand Down
2 changes: 2 additions & 0 deletions packages/react-router/tests/useBlocker.test-d.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ test('shouldBlockFn has corrent action', () => {
.toHaveProperty('shouldBlockFn')
.parameter(0)
.toHaveProperty('current')
.exclude(undefined)
.toHaveProperty('routeId')
.toEqualTypeOf<'__root__' | '/' | '/invoices' | '/invoices/'>()

Expand All @@ -117,6 +118,7 @@ test('shouldBlockFn has corrent action', () => {
.toHaveProperty('shouldBlockFn')
.parameter(0)
.toHaveProperty('next')
.exclude(undefined)
.toHaveProperty('routeId')
.toEqualTypeOf<'__root__' | '/' | '/invoices' | '/invoices/'>()
})
59 changes: 57 additions & 2 deletions packages/react-router/tests/useBlocker.test.tsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
import React from 'react'
import '@testing-library/jest-dom/vitest'
import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'
import { cleanup, fireEvent, render, screen } from '@testing-library/react'
import {
cleanup,
fireEvent,
render,
screen,
} from '@testing-library/react'

import { z } from 'zod'
import {
Link,
Outlet,
RouterProvider,
createBrowserHistory,
createRootRoute,
Expand Down Expand Up @@ -360,7 +367,7 @@ describe('useBlocker', () => {

useBlocker<Router>({
shouldBlockFn: ({ next }) => {
if (next.fullPath === '/posts') {
if (next?.fullPath === '/posts') {
return true
}
return false
Expand Down Expand Up @@ -440,4 +447,52 @@ describe('useBlocker', () => {

expect(window.location.pathname).toBe('/invoices')
})

test('does not throw when navigating to an unknown route and throwOnUnknownRoute is false', async () => {
const rootRoute = createRootRoute({
component:() => <Outlet />,
notFoundComponent: () => <h1 data-testid="not-found">Not Found</h1>,
})

const IndexComponent = () => {
useBlocker({
shouldBlockFn: () => false,
throwOnUnknownRoute: false
})

return (
<>
<h1>Index</h1>
<Link data-testid='unknown-link' to='/unknown'>
Unknown
</Link>
</>
)
}

const indexRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/',
component: IndexComponent,
})

const router = createRouter({
routeTree: rootRoute.addChildren([indexRoute]),
history,
})

render(<RouterProvider router={router} />)
await router.load()
await screen.findByTestId('unknown-link')

const unknownLink = screen.getByTestId('unknown-link')
unknownLink.click()

const notFoundComponent = await screen.findByTestId(
'not-found',
{},
{ timeout: 1000 },
)
expect(notFoundComponent).toBeInTheDocument()
})
})
25 changes: 19 additions & 6 deletions packages/solid-router/src/useBlocker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ type MakeShouldBlockFnLocationUnion<
type BlockerResolver<TRouter extends AnyRouter = RegisteredRouter> =
| {
status: 'blocked'
current: MakeShouldBlockFnLocationUnion<TRouter>
next: MakeShouldBlockFnLocationUnion<TRouter>
current: MakeShouldBlockFnLocationUnion<TRouter> | undefined
next: MakeShouldBlockFnLocationUnion<TRouter> | undefined
action: HistoryAction
proceed: () => void
reset: () => void
Expand All @@ -58,8 +58,8 @@ type BlockerResolver<TRouter extends AnyRouter = RegisteredRouter> =
}

type ShouldBlockFnArgs<TRouter extends AnyRouter = RegisteredRouter> = {
current: MakeShouldBlockFnLocationUnion<TRouter>
next: MakeShouldBlockFnLocationUnion<TRouter>
current: MakeShouldBlockFnLocationUnion<TRouter> | undefined
next: MakeShouldBlockFnLocationUnion<TRouter> | undefined
action: HistoryAction
}

Expand All @@ -74,6 +74,7 @@ export type UseBlockerOpts<
enableBeforeUnload?: boolean | (() => boolean)
disabled?: boolean
withResolver?: TWithResolver
throwOnUnknownRoute?: boolean
}

type LegacyBlockerFn = () => Promise<any> | any
Expand Down Expand Up @@ -165,6 +166,7 @@ export function useBlocker(
enableBeforeUnload: true,
disabled: false,
withResolver: false,
throwOnUnknownRoute: true,
},
_resolveBlockerOpts(opts, condition),
)
Expand All @@ -184,14 +186,14 @@ export function useBlocker(
const blockerFnComposed = async (blockerFnArgs: BlockerFnArgs) => {
function getLocation(
location: HistoryLocation,
): AnyShouldBlockFnLocation {
): AnyShouldBlockFnLocation | undefined {
const parsedLocation = router.parseLocation(location)
const matchedRoutes = router.getMatchedRoutes(
parsedLocation.pathname,
undefined,
)
if (matchedRoutes.foundRoute === undefined) {
throw new Error(`No route found for location ${location.href}`)
return undefined
}
return {
routeId: matchedRoutes.foundRoute.id,
Expand All @@ -203,7 +205,18 @@ export function useBlocker(
}

const current = getLocation(blockerFnArgs.currentLocation)
if (!current && props.throwOnUnknownRoute) {
throw new Error(
`No route found for location ${blockerFnArgs.currentLocation.href}`,
)
}

const next = getLocation(blockerFnArgs.nextLocation)
if (!next && props.throwOnUnknownRoute) {
throw new Error(
`No route found for location ${blockerFnArgs.nextLocation.href}`,
)
}

const shouldBlock = await props.shouldBlockFn({
action: blockerFnArgs.action,
Expand Down
2 changes: 2 additions & 0 deletions packages/solid-router/tests/useBlocker.test-d.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ test('shouldBlockFn has corrent action', () => {
.toHaveProperty('shouldBlockFn')
.parameter(0)
.toHaveProperty('current')
.exclude(undefined)
.toHaveProperty('routeId')
.toEqualTypeOf<'__root__' | '/' | '/invoices' | '/invoices/'>()

Expand All @@ -114,6 +115,7 @@ test('shouldBlockFn has corrent action', () => {
.toHaveProperty('shouldBlockFn')
.parameter(0)
.toHaveProperty('next')
.exclude(undefined)
.toHaveProperty('routeId')
.toEqualTypeOf<'__root__' | '/' | '/invoices' | '/invoices/'>()
})
Loading