Skip to content

Commit

Permalink
Fix Router Error Events in Shallow Routing by Skipping cancelHandler …
Browse files Browse the repository at this point in the history
…Creation (#61771)

### Problem

We've identified a bug within Next.js's pages router when utilizing the
shallow routing API, specifically when invoking `router.push` with the
`{ shallow: true }` option, like so:
 
```javascript
router.push('/?counter=10', undefined, { shallow: true });
```

Shallow routing is designed to update the URL without running data
fetching methods such as getServerSideProps, getStaticProps, or
getInitialProps. However, a side effect of this process is that it skips
the clean-up of the cancelHandler. This leads to router error events
being fired erroneously, causing confusion and potential stability
issues, as the system behaves as if an error occurred when, in fact,
none did.
 
### Solution

This PR addresses the issue by modifying the shallow routing logic to
also skip the creation of the cancelHandler. Given that shallow routing
operations are synchronous and do not involve data fetching or other
asynchronous tasks that might need to be canceled, the cancelHandler is
unnecessary in this context.

fixes #61772

---------

Co-authored-by: Shu Ding <g@shud.in>
  • Loading branch information
rvetere and shuding authored Feb 28, 2024
1 parent 6b6575c commit 3790099
Show file tree
Hide file tree
Showing 3 changed files with 60 additions and 2 deletions.
4 changes: 2 additions & 2 deletions packages/next/src/shared/lib/router/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1943,13 +1943,13 @@ export default class Router implements BaseRouter {
let route = requestedRoute

try {
const handleCancelled = getCancelledHandler({ route, router: this })

let existingInfo: PrivateRouteInfo | undefined = this.components[route]
if (routeProps.shallow && existingInfo && this.route === route) {
return existingInfo
}

const handleCancelled = getCancelledHandler({ route, router: this })

if (hasMiddleware) {
existingInfo = undefined
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import Link from 'next/link'
import { useRouter } from 'next/router'
import { useEffect, useState } from 'react'

export default function Page() {
const router = useRouter()

const [routeState, setRouteResult] = useState({
completed: 0,
errors: 0,
})
useEffect(() => {
const increaseErrorCount = () =>
setRouteResult((state) => ({ ...state, errors: state.errors + 1 }))
const increaseRouteComplete = () =>
setRouteResult((state) => ({ ...state, completed: state.completed + 1 }))
router.events.on('routeChangeError', increaseErrorCount)
router.events.on('routeChangeComplete', increaseRouteComplete)

return () => {
router.events.off('routeChangeError', increaseErrorCount)
router.events.off('routeChangeComplete', increaseRouteComplete)
}
})

return (
<>
<div id="routeState">{JSON.stringify(routeState)}</div>
<Link href="?prop=foo" id="link" shallow={true}>
Click me
</Link>
<button
id="router-replace"
onClick={() => {
router.replace('?prop=baz', undefined, { shallow: true })
}}
>
Push me
</button>
</>
)
}
16 changes: 16 additions & 0 deletions test/development/pages-dir/client-navigation/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1114,6 +1114,22 @@ createNextDescribe(
await browser.close()
}
})

it('router.replace with shallow=true shall not throw route cancelled errors', async () => {
const browser = await webdriver(next.appPort, '/nav/query-only-shallow')
try {
await browser.elementByCss('#router-replace').click()
// the error occurs on every replace() after the first one
await browser.elementByCss('#router-replace').click()

await check(
() => browser.waitForElementByCss('#routeState').text(),
'{"completed":2,"errors":0}'
)
} finally {
await browser.close()
}
})
})

describe('with getInitialProp redirect', () => {
Expand Down

0 comments on commit 3790099

Please sign in to comment.