From ebaeaa80394965e95da65678024fdb96c8c4545f Mon Sep 17 00:00:00 2001 From: Tim Neutkens Date: Tue, 16 May 2023 14:37:58 +0200 Subject: [PATCH 1/5] Reset not-found and error boundary when navigating --- .../src/client/components/error-boundary.tsx | 29 ++++++++++++--- .../client/components/not-found-boundary.tsx | 36 ++++++++++++++++--- .../navigation/app/not-found/not-found.js | 12 +++++-- .../navigation/app/not-found/result/page.js | 3 ++ .../app/not-found/servercomponent/page.js | 1 - .../app/not-found/with-link/page.js | 8 +++++ .../app-dir/not-found-linking/app/layout.tsx | 7 ++++ .../not-found-linking/app/not-found.tsx | 12 +++++++ .../app-dir/not-found-linking/app/page.tsx | 3 ++ .../not-found-linking/app/result/page.tsx | 3 ++ .../app/trigger-404/page.tsx | 7 ++++ .../app-dir/not-found-linking/next.config.js | 10 ++++++ .../not-found-linking.test.ts | 24 +++++++++++++ 13 files changed, 143 insertions(+), 12 deletions(-) create mode 100644 test/e2e/app-dir/navigation/app/not-found/result/page.js create mode 100644 test/e2e/app-dir/navigation/app/not-found/with-link/page.js create mode 100644 test/e2e/app-dir/not-found-linking/app/layout.tsx create mode 100644 test/e2e/app-dir/not-found-linking/app/not-found.tsx create mode 100644 test/e2e/app-dir/not-found-linking/app/page.tsx create mode 100644 test/e2e/app-dir/not-found-linking/app/result/page.tsx create mode 100644 test/e2e/app-dir/not-found-linking/app/trigger-404/page.tsx create mode 100644 test/e2e/app-dir/not-found-linking/next.config.js create mode 100644 test/e2e/app-dir/not-found-linking/not-found-linking.test.ts diff --git a/packages/next/src/client/components/error-boundary.tsx b/packages/next/src/client/components/error-boundary.tsx index 02b6e0fb24e78..d5cf12044ba20 100644 --- a/packages/next/src/client/components/error-boundary.tsx +++ b/packages/next/src/client/components/error-boundary.tsx @@ -1,6 +1,7 @@ 'use client' import React from 'react' +import { usePathname } from './navigation' const styles = { error: { @@ -36,13 +37,17 @@ export interface ErrorBoundaryProps { errorStyles?: React.ReactNode | undefined } +interface ErrorBoundaryHandlerProps extends ErrorBoundaryProps { + pathname: string +} + export class ErrorBoundaryHandler extends React.Component< - ErrorBoundaryProps, - { error: Error | null } + ErrorBoundaryHandlerProps, + { error: Error | null; previousPathname: string } > { - constructor(props: ErrorBoundaryProps) { + constructor(props: ErrorBoundaryHandlerProps) { super(props) - this.state = { error: null } + this.state = { error: null, previousPathname: this.props.pathname } } static getDerivedStateFromError(error: Error) { @@ -55,6 +60,20 @@ export class ErrorBoundaryHandler extends React.Component< render() { if (this.state.error) { + /** + * Handles reset of the error boundary when a navigation happens. + * Ensures the error boundary does not stay enabled when navigating to a new page. + * Approach of setState in render is safe as it checks the previous pathname and then overrides + * it as outlined in https://react.dev/reference/react/useState#storing-information-from-previous-renders + */ + if (this.props.pathname !== this.state.previousPathname) { + this.setState((_state) => { + return { + error: null, + previousPathname: this.props.pathname, + } + }) + } return ( <> {this.props.errorStyles} @@ -105,9 +124,11 @@ export function ErrorBoundary({ errorStyles, children, }: ErrorBoundaryProps & { children: React.ReactNode }): JSX.Element { + const pathname = usePathname() if (errorComponent) { return ( diff --git a/packages/next/src/client/components/not-found-boundary.tsx b/packages/next/src/client/components/not-found-boundary.tsx index 81126b2e3f9c7..f33c4e0e440ab 100644 --- a/packages/next/src/client/components/not-found-boundary.tsx +++ b/packages/next/src/client/components/not-found-boundary.tsx @@ -1,4 +1,5 @@ import React from 'react' +import { usePathname } from './navigation' interface NotFoundBoundaryProps { notFound?: React.ReactNode @@ -7,13 +8,20 @@ interface NotFoundBoundaryProps { children: React.ReactNode } +interface NotFoundErrorBoundaryProps extends NotFoundBoundaryProps { + pathname: string +} + class NotFoundErrorBoundary extends React.Component< - NotFoundBoundaryProps, - { notFoundTriggered: boolean } + NotFoundErrorBoundaryProps, + { notFoundTriggered: boolean; previousPathname: string } > { - constructor(props: NotFoundBoundaryProps) { + constructor(props: NotFoundErrorBoundaryProps) { super(props) - this.state = { notFoundTriggered: !!props.asNotFound } + this.state = { + notFoundTriggered: !!props.asNotFound, + previousPathname: props.pathname, + } } static getDerivedStateFromError(error: any) { @@ -26,6 +34,24 @@ class NotFoundErrorBoundary extends React.Component< render() { if (this.state.notFoundTriggered) { + /** + * Handles reset of the error boundary when a navigation happens. + * Ensures the error boundary does not stay enabled when navigating to a new page. + * Approach of setState in render is safe as it checks the previous pathname and then overrides + * it as outlined in https://react.dev/reference/react/useState#storing-information-from-previous-renders + */ + if ( + this.props.pathname !== this.state.previousPathname && + this.state.notFoundTriggered + ) { + this.setState((_state) => { + return { + notFoundTriggered: false, + previousPathname: this.props.pathname, + } + }) + } + return ( <> @@ -45,8 +71,10 @@ export function NotFoundBoundary({ asNotFound, children, }: NotFoundBoundaryProps) { + const pathname = usePathname() return notFound ? ( - Not Found! - + <> +

+ Not Found! +

+ + To Result + + ) } diff --git a/test/e2e/app-dir/navigation/app/not-found/result/page.js b/test/e2e/app-dir/navigation/app/not-found/result/page.js new file mode 100644 index 0000000000000..f99fc856f67a8 --- /dev/null +++ b/test/e2e/app-dir/navigation/app/not-found/result/page.js @@ -0,0 +1,3 @@ +export default function Page() { + return

Result Page!

+} diff --git a/test/e2e/app-dir/navigation/app/not-found/servercomponent/page.js b/test/e2e/app-dir/navigation/app/not-found/servercomponent/page.js index 180403e6e0140..d3cf49459932a 100644 --- a/test/e2e/app-dir/navigation/app/not-found/servercomponent/page.js +++ b/test/e2e/app-dir/navigation/app/not-found/servercomponent/page.js @@ -1,4 +1,3 @@ -// TODO-APP: enable when flight error serialization is implemented import { notFound } from 'next/navigation' export default function Page() { diff --git a/test/e2e/app-dir/navigation/app/not-found/with-link/page.js b/test/e2e/app-dir/navigation/app/not-found/with-link/page.js new file mode 100644 index 0000000000000..fd2d092212385 --- /dev/null +++ b/test/e2e/app-dir/navigation/app/not-found/with-link/page.js @@ -0,0 +1,8 @@ +import { notFound } from 'next/navigation' + +export const dynamic = 'force-dynamic' + +export default function Page() { + notFound() + return <> +} diff --git a/test/e2e/app-dir/not-found-linking/app/layout.tsx b/test/e2e/app-dir/not-found-linking/app/layout.tsx new file mode 100644 index 0000000000000..e7077399c03ce --- /dev/null +++ b/test/e2e/app-dir/not-found-linking/app/layout.tsx @@ -0,0 +1,7 @@ +export default function Root({ children }: { children: React.ReactNode }) { + return ( + + {children} + + ) +} diff --git a/test/e2e/app-dir/not-found-linking/app/not-found.tsx b/test/e2e/app-dir/not-found-linking/app/not-found.tsx new file mode 100644 index 0000000000000..50a433f5e4431 --- /dev/null +++ b/test/e2e/app-dir/not-found-linking/app/not-found.tsx @@ -0,0 +1,12 @@ +import Link from 'next/link' + +export default function NotFound() { + return ( + <> +

Not Found!

+ + To Result + + + ) +} diff --git a/test/e2e/app-dir/not-found-linking/app/page.tsx b/test/e2e/app-dir/not-found-linking/app/page.tsx new file mode 100644 index 0000000000000..c0c3e311375c7 --- /dev/null +++ b/test/e2e/app-dir/not-found-linking/app/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return

Home

+} diff --git a/test/e2e/app-dir/not-found-linking/app/result/page.tsx b/test/e2e/app-dir/not-found-linking/app/result/page.tsx new file mode 100644 index 0000000000000..f99fc856f67a8 --- /dev/null +++ b/test/e2e/app-dir/not-found-linking/app/result/page.tsx @@ -0,0 +1,3 @@ +export default function Page() { + return

Result Page!

+} diff --git a/test/e2e/app-dir/not-found-linking/app/trigger-404/page.tsx b/test/e2e/app-dir/not-found-linking/app/trigger-404/page.tsx new file mode 100644 index 0000000000000..bdcc212a66a02 --- /dev/null +++ b/test/e2e/app-dir/not-found-linking/app/trigger-404/page.tsx @@ -0,0 +1,7 @@ +import { notFound } from 'next/navigation' + +export const dynamic = 'force-dynamic' + +export default function Page() { + notFound() +} diff --git a/test/e2e/app-dir/not-found-linking/next.config.js b/test/e2e/app-dir/not-found-linking/next.config.js new file mode 100644 index 0000000000000..3d24f6813557e --- /dev/null +++ b/test/e2e/app-dir/not-found-linking/next.config.js @@ -0,0 +1,10 @@ +/** + * @type {import('next').NextConfig} + */ +const nextConfig = { + typescript: { + ignoreBuildErrors: true, + }, +} + +module.exports = nextConfig diff --git a/test/e2e/app-dir/not-found-linking/not-found-linking.test.ts b/test/e2e/app-dir/not-found-linking/not-found-linking.test.ts new file mode 100644 index 0000000000000..5e8bb2cb82f87 --- /dev/null +++ b/test/e2e/app-dir/not-found-linking/not-found-linking.test.ts @@ -0,0 +1,24 @@ +import { createNextDescribe } from 'e2e-utils' + +createNextDescribe( + 'not-found-linking', + { + files: __dirname, + }, + ({ next }) => { + it('should allow navigation on not-found', async () => { + const browser = await next.browser('/trigger-404') + expect(await browser.elementByCss('#not-found-component').text()).toBe( + 'Not Found!' + ) + + expect( + await browser + .elementByCss('#to-result') + .click() + .waitForElementByCss('#result-page') + .text() + ).toBe('Result Page!') + }) + } +) From 6aa43498c6a00a3cab9b7d0dbfd37a637025d28e Mon Sep 17 00:00:00 2001 From: Tim Neutkens Date: Tue, 16 May 2023 15:15:11 +0200 Subject: [PATCH 2/5] Add test for error boundary --- .../app/error.tsx | 13 +++++++++++++ .../app/layout.tsx | 0 .../app/not-found.tsx | 0 .../app/page.tsx | 0 .../app/result/page.tsx | 0 .../app/trigger-404/page.tsx | 0 .../app/trigger-error/page.tsx | 5 +++++ .../error-boundary-and-not-found-linking.test.ts} | 15 +++++++++++++++ .../next.config.js | 0 9 files changed, 33 insertions(+) create mode 100644 test/e2e/app-dir/error-boundary-and-not-found-linking/app/error.tsx rename test/e2e/app-dir/{not-found-linking => error-boundary-and-not-found-linking}/app/layout.tsx (100%) rename test/e2e/app-dir/{not-found-linking => error-boundary-and-not-found-linking}/app/not-found.tsx (100%) rename test/e2e/app-dir/{not-found-linking => error-boundary-and-not-found-linking}/app/page.tsx (100%) rename test/e2e/app-dir/{not-found-linking => error-boundary-and-not-found-linking}/app/result/page.tsx (100%) rename test/e2e/app-dir/{not-found-linking => error-boundary-and-not-found-linking}/app/trigger-404/page.tsx (100%) create mode 100644 test/e2e/app-dir/error-boundary-and-not-found-linking/app/trigger-error/page.tsx rename test/e2e/app-dir/{not-found-linking/not-found-linking.test.ts => error-boundary-and-not-found-linking/error-boundary-and-not-found-linking.test.ts} (57%) rename test/e2e/app-dir/{not-found-linking => error-boundary-and-not-found-linking}/next.config.js (100%) diff --git a/test/e2e/app-dir/error-boundary-and-not-found-linking/app/error.tsx b/test/e2e/app-dir/error-boundary-and-not-found-linking/app/error.tsx new file mode 100644 index 0000000000000..2d203c0ac82cd --- /dev/null +++ b/test/e2e/app-dir/error-boundary-and-not-found-linking/app/error.tsx @@ -0,0 +1,13 @@ +'use client' +import Link from 'next/link' + +export default function ErrorComponent() { + return ( + <> +

Error Happened!

+ + To Result + + + ) +} diff --git a/test/e2e/app-dir/not-found-linking/app/layout.tsx b/test/e2e/app-dir/error-boundary-and-not-found-linking/app/layout.tsx similarity index 100% rename from test/e2e/app-dir/not-found-linking/app/layout.tsx rename to test/e2e/app-dir/error-boundary-and-not-found-linking/app/layout.tsx diff --git a/test/e2e/app-dir/not-found-linking/app/not-found.tsx b/test/e2e/app-dir/error-boundary-and-not-found-linking/app/not-found.tsx similarity index 100% rename from test/e2e/app-dir/not-found-linking/app/not-found.tsx rename to test/e2e/app-dir/error-boundary-and-not-found-linking/app/not-found.tsx diff --git a/test/e2e/app-dir/not-found-linking/app/page.tsx b/test/e2e/app-dir/error-boundary-and-not-found-linking/app/page.tsx similarity index 100% rename from test/e2e/app-dir/not-found-linking/app/page.tsx rename to test/e2e/app-dir/error-boundary-and-not-found-linking/app/page.tsx diff --git a/test/e2e/app-dir/not-found-linking/app/result/page.tsx b/test/e2e/app-dir/error-boundary-and-not-found-linking/app/result/page.tsx similarity index 100% rename from test/e2e/app-dir/not-found-linking/app/result/page.tsx rename to test/e2e/app-dir/error-boundary-and-not-found-linking/app/result/page.tsx diff --git a/test/e2e/app-dir/not-found-linking/app/trigger-404/page.tsx b/test/e2e/app-dir/error-boundary-and-not-found-linking/app/trigger-404/page.tsx similarity index 100% rename from test/e2e/app-dir/not-found-linking/app/trigger-404/page.tsx rename to test/e2e/app-dir/error-boundary-and-not-found-linking/app/trigger-404/page.tsx diff --git a/test/e2e/app-dir/error-boundary-and-not-found-linking/app/trigger-error/page.tsx b/test/e2e/app-dir/error-boundary-and-not-found-linking/app/trigger-error/page.tsx new file mode 100644 index 0000000000000..5fca91341f62b --- /dev/null +++ b/test/e2e/app-dir/error-boundary-and-not-found-linking/app/trigger-error/page.tsx @@ -0,0 +1,5 @@ +export const dynamic = 'force-dynamic' + +export default function Page() { + throw new Error('This is an error') +} diff --git a/test/e2e/app-dir/not-found-linking/not-found-linking.test.ts b/test/e2e/app-dir/error-boundary-and-not-found-linking/error-boundary-and-not-found-linking.test.ts similarity index 57% rename from test/e2e/app-dir/not-found-linking/not-found-linking.test.ts rename to test/e2e/app-dir/error-boundary-and-not-found-linking/error-boundary-and-not-found-linking.test.ts index 5e8bb2cb82f87..3e602154f8149 100644 --- a/test/e2e/app-dir/not-found-linking/not-found-linking.test.ts +++ b/test/e2e/app-dir/error-boundary-and-not-found-linking/error-boundary-and-not-found-linking.test.ts @@ -20,5 +20,20 @@ createNextDescribe( .text() ).toBe('Result Page!') }) + + it('should allow navigation on error', async () => { + const browser = await next.browser('/trigger-error') + expect(await browser.elementByCss('#error-component').text()).toBe( + 'Error Happened!' + ) + + expect( + await browser + .elementByCss('#to-result') + .click() + .waitForElementByCss('#result-page') + .text() + ).toBe('Result Page!') + }) } ) diff --git a/test/e2e/app-dir/not-found-linking/next.config.js b/test/e2e/app-dir/error-boundary-and-not-found-linking/next.config.js similarity index 100% rename from test/e2e/app-dir/not-found-linking/next.config.js rename to test/e2e/app-dir/error-boundary-and-not-found-linking/next.config.js From af832a4a973175b41c1402e7bf4bff7a4494bed1 Mon Sep 17 00:00:00 2001 From: Tim Neutkens Date: Wed, 17 May 2023 14:21:16 +0200 Subject: [PATCH 3/5] Use getDerivedStateFromProps instead as class components don't support setState in render --- .../src/client/components/error-boundary.tsx | 26 ++++++++++- .../client/components/not-found-boundary.tsx | 44 +++++++++++-------- 2 files changed, 50 insertions(+), 20 deletions(-) diff --git a/packages/next/src/client/components/error-boundary.tsx b/packages/next/src/client/components/error-boundary.tsx index d5cf12044ba20..44e407c048dae 100644 --- a/packages/next/src/client/components/error-boundary.tsx +++ b/packages/next/src/client/components/error-boundary.tsx @@ -41,9 +41,14 @@ interface ErrorBoundaryHandlerProps extends ErrorBoundaryProps { pathname: string } +interface ErrorBoundaryHandlerState { + error: Error | null + previousPathname: string +} + export class ErrorBoundaryHandler extends React.Component< ErrorBoundaryHandlerProps, - { error: Error | null; previousPathname: string } + ErrorBoundaryHandlerState > { constructor(props: ErrorBoundaryHandlerProps) { super(props) @@ -54,6 +59,25 @@ export class ErrorBoundaryHandler extends React.Component< return { error } } + static getDerivedStateFromProps( + props: ErrorBoundaryHandlerProps, + state: ErrorBoundaryHandlerState + ): ErrorBoundaryHandlerState | null { + /** + * Handles reset of the error boundary when a navigation happens. + * Ensures the error boundary does not stay enabled when navigating to a new page. + * Approach of setState in render is safe as it checks the previous pathname and then overrides + * it as outlined in https://react.dev/reference/react/useState#storing-information-from-previous-renders + */ + if (props.pathname !== state.previousPathname && state.error) { + return { + error: null, + previousPathname: props.pathname, + } + } + return null + } + reset = () => { this.setState({ error: null }) } diff --git a/packages/next/src/client/components/not-found-boundary.tsx b/packages/next/src/client/components/not-found-boundary.tsx index f33c4e0e440ab..a0a36ae1b3660 100644 --- a/packages/next/src/client/components/not-found-boundary.tsx +++ b/packages/next/src/client/components/not-found-boundary.tsx @@ -12,9 +12,14 @@ interface NotFoundErrorBoundaryProps extends NotFoundBoundaryProps { pathname: string } +interface NotFoundErrorBoundaryState { + notFoundTriggered: boolean + previousPathname: string +} + class NotFoundErrorBoundary extends React.Component< NotFoundErrorBoundaryProps, - { notFoundTriggered: boolean; previousPathname: string } + NotFoundErrorBoundaryState > { constructor(props: NotFoundErrorBoundaryProps) { super(props) @@ -32,26 +37,27 @@ class NotFoundErrorBoundary extends React.Component< throw error } - render() { - if (this.state.notFoundTriggered) { - /** - * Handles reset of the error boundary when a navigation happens. - * Ensures the error boundary does not stay enabled when navigating to a new page. - * Approach of setState in render is safe as it checks the previous pathname and then overrides - * it as outlined in https://react.dev/reference/react/useState#storing-information-from-previous-renders - */ - if ( - this.props.pathname !== this.state.previousPathname && - this.state.notFoundTriggered - ) { - this.setState((_state) => { - return { - notFoundTriggered: false, - previousPathname: this.props.pathname, - } - }) + static getDerivedStateFromProps( + props: NotFoundErrorBoundaryProps, + state: NotFoundErrorBoundaryState + ): NotFoundErrorBoundaryState | null { + /** + * Handles reset of the error boundary when a navigation happens. + * Ensures the error boundary does not stay enabled when navigating to a new page. + * Approach of setState in render is safe as it checks the previous pathname and then overrides + * it as outlined in https://react.dev/reference/react/useState#storing-information-from-previous-renders + */ + if (props.pathname !== state.previousPathname && state.notFoundTriggered) { + return { + notFoundTriggered: false, + previousPathname: props.pathname, } + } + return null + } + render() { + if (this.state.notFoundTriggered) { return ( <> From fb40ea49870c3e17f845f32d5e545c39636d826f Mon Sep 17 00:00:00 2001 From: Tim Neutkens Date: Thu, 18 May 2023 14:58:28 +0200 Subject: [PATCH 4/5] Remove outdated code --- .../next/src/client/components/error-boundary.tsx | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/packages/next/src/client/components/error-boundary.tsx b/packages/next/src/client/components/error-boundary.tsx index 44e407c048dae..d1426e3de4607 100644 --- a/packages/next/src/client/components/error-boundary.tsx +++ b/packages/next/src/client/components/error-boundary.tsx @@ -84,20 +84,6 @@ export class ErrorBoundaryHandler extends React.Component< render() { if (this.state.error) { - /** - * Handles reset of the error boundary when a navigation happens. - * Ensures the error boundary does not stay enabled when navigating to a new page. - * Approach of setState in render is safe as it checks the previous pathname and then overrides - * it as outlined in https://react.dev/reference/react/useState#storing-information-from-previous-renders - */ - if (this.props.pathname !== this.state.previousPathname) { - this.setState((_state) => { - return { - error: null, - previousPathname: this.props.pathname, - } - }) - } return ( <> {this.props.errorStyles} From 5a928332d48b8d767a55d682209ef260d926a8c9 Mon Sep 17 00:00:00 2001 From: Tim Neutkens Date: Thu, 18 May 2023 18:36:26 +0200 Subject: [PATCH 5/5] Ensure previousPathName is always set based on props --- packages/next/src/client/components/error-boundary.tsx | 5 ++++- packages/next/src/client/components/not-found-boundary.tsx | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/next/src/client/components/error-boundary.tsx b/packages/next/src/client/components/error-boundary.tsx index d1426e3de4607..bc83c846ff743 100644 --- a/packages/next/src/client/components/error-boundary.tsx +++ b/packages/next/src/client/components/error-boundary.tsx @@ -75,7 +75,10 @@ export class ErrorBoundaryHandler extends React.Component< previousPathname: props.pathname, } } - return null + return { + error: state.error, + previousPathname: props.pathname, + } } reset = () => { diff --git a/packages/next/src/client/components/not-found-boundary.tsx b/packages/next/src/client/components/not-found-boundary.tsx index a0a36ae1b3660..cf594deecba46 100644 --- a/packages/next/src/client/components/not-found-boundary.tsx +++ b/packages/next/src/client/components/not-found-boundary.tsx @@ -53,7 +53,10 @@ class NotFoundErrorBoundary extends React.Component< previousPathname: props.pathname, } } - return null + return { + notFoundTriggered: state.notFoundTriggered, + previousPathname: props.pathname, + } } render() {