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

Reland "Refine the not-found rendering process for app router" #52985

Merged
merged 10 commits into from
Jul 21, 2023
23 changes: 2 additions & 21 deletions packages/next-swc/crates/next-core/js/src/dev/hot-reloader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,12 @@ import { useRouter, usePathname } from 'next/dist/client/components/navigation'
import { useEffect } from 'react'
import { subscribeToUpdate } from '@vercel/turbopack-ecmascript-runtime/dev/client/hmr-client'
import { ReactDevOverlay } from './client'
import { NotFoundBoundary } from 'next/dist/client/components/not-found-boundary'

type HotReloadProps = React.PropsWithChildren<{
assetPrefix?: string
notFound?: React.ReactNode
notFoundStyles?: React.ReactNode
asNotFound?: boolean
}>

export default function HotReload({
assetPrefix,
children,
notFound,
notFoundStyles,
asNotFound,
}: HotReloadProps) {
export default function HotReload({ children }: HotReloadProps) {
const router = useRouter()
const path = usePathname()!.slice(1)

Expand All @@ -41,14 +31,5 @@ export default function HotReload({
return unsubscribe
}, [router, path])

return (
<NotFoundBoundary
key={asNotFound + ''}
notFound={notFound}
notFoundStyles={notFoundStyles}
asNotFound={asNotFound}
>
<ReactDevOverlay globalOverlay={true}>{children}</ReactDevOverlay>
</NotFoundBoundary>
)
return <ReactDevOverlay globalOverlay={true}>{children}</ReactDevOverlay>
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { ErrorBoundary } from './ErrorBoundary'
import { Base } from './styles/Base'
import { ComponentStyles } from './styles/ComponentStyles'
import { CssReset } from './styles/CssReset'
import { notFound } from 'next/dist/client/components/not-found'

type RefreshState =
| {
Expand Down Expand Up @@ -210,10 +209,6 @@ export default function ReactDevOverlay({

const isMounted = hasBuildError || hasRuntimeErrors

if (state.notFound) {
notFound()
}

return (
<React.Fragment>
<ErrorBoundary
Expand Down
2 changes: 1 addition & 1 deletion packages/next-swc/crates/next-core/src/app_source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,7 @@ pub async fn create_app_source(
)))
.collect();

if let Some(&Entrypoint::AppPage { loader_tree }) = entrypoints.get("/") {
if let Some(&Entrypoint::AppPage { loader_tree }) = entrypoints.get("/_not-found") {
if loader_tree.await?.components.await?.not_found.is_some() {
// Only add a source for the app 404 page if a top-level not-found page is
// defined. Otherwise, the 404 page is handled by the pages logic.
Expand Down
15 changes: 11 additions & 4 deletions packages/next-swc/crates/next-core/src/app_structure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use turbopack_binding::{
turbopack::core::issue::{Issue, IssueExt, IssueSeverity},
};

use crate::next_config::NextConfig;
use crate::{next_config::NextConfig, next_import_map::get_next_package};

/// A final route in the app directory.
#[turbo_tasks::value]
Expand Down Expand Up @@ -675,9 +675,16 @@ async fn directory_tree_to_entrypoints_internal(
LoaderTree {
segment: directory_name.to_string(),
parallel_routes: indexmap! {
// TODO(alexkirsz) Next.js has a __DEFAULT__ entry for
// next/dist/client/components/parallel-route-default
// here.
"children".to_string() => LoaderTree {
segment: "__DEFAULT__".to_string(),
parallel_routes: IndexMap::new(),
components: Components {
default: Some(get_next_package(app_dir).join("dist/client/components/parallel-route-default.js".to_string())),
..Default::default()
}
.cell(),
}
.cell(),
},
components: components.without_leafs().cell(),
}
Expand Down
13 changes: 11 additions & 2 deletions packages/next-swc/crates/next-dev-tests/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#![cfg(test)]

use std::{
collections::{hash_map::Entry, HashMap},
env,
fmt::Write,
future::{pending, Future},
Expand Down Expand Up @@ -34,6 +35,7 @@ use next_core::turbopack::{
};
use next_dev::{EntryRequest, NextDevServerBuilder};
use owo_colors::OwoColorize;
use parking_lot::Mutex;
use regex::{Captures, Regex, Replacer};
use serde::Deserialize;
use tempdir::TempDir;
Expand Down Expand Up @@ -637,10 +639,12 @@ struct ChangeFileCommand {
replace_with: String,
}

#[turbo_tasks::value(shared)]
#[turbo_tasks::value(shared, serialization = "none", eq = "manual", cell = "new")]
struct TestIssueReporter {
#[turbo_tasks(trace_ignore, debug_ignore)]
pub issue_tx: State<UnboundedSender<(ReadRef<PlainIssue>, ReadRef<ValueDebugString>)>>,
#[turbo_tasks(trace_ignore, debug_ignore)]
pub already_printed: Mutex<HashMap<String, ()>>,
}

#[turbo_tasks::value_impl]
Expand All @@ -653,6 +657,7 @@ impl TestIssueReporter {
) -> Vc<Self> {
TestIssueReporter {
issue_tx: State::new((*issue_tx).clone()),
already_printed: Default::default(),
}
.cell()
}
Expand All @@ -678,7 +683,11 @@ impl IssueReporter for TestIssueReporter {
for (issue, path) in captured_issues.iter_with_shortest_path() {
let plain = NormalizedIssue(issue).cell().into_plain(path);
issue_tx.send((plain.await?, plain.dbg().await?))?;
println!("{}", format_issue(&*plain.await?, None, &log_options));
let str = format_issue(&*plain.await?, None, &log_options);
if let Entry::Vacant(e) = self.already_printed.lock().entry(str) {
println!("{}", e.key());
e.insert(());
}
}
Ok(Vc::cell(false))
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ function runTests(harness: Harness, iframe: HTMLIFrameElement) {
TIMEOUT
)

// TODO: This test is flaky, so it needs a particularly long timeout.
it(
'renders a custom 404 page',
async () => {
Expand All @@ -58,7 +59,7 @@ function runTests(harness: Harness, iframe: HTMLIFrameElement) {
iframe.contentDocument!.querySelector('[data-test-notfound]')
).not.toBeNull()
},
TIMEOUT
LONG_TIMEOUT
)

// TODO: This test is flaky, so it needs a particularly long timeout.
Expand Down
41 changes: 14 additions & 27 deletions packages/next/src/client/components/app-router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ import { isBot } from '../../shared/lib/router/utils/is-bot'
import { addBasePath } from '../add-base-path'
import { AppRouterAnnouncer } from './app-router-announcer'
import { RedirectBoundary } from './redirect-boundary'
import { NotFoundBoundary } from './not-found-boundary'
import { findHeadInCache } from './router-reducer/reducers/find-head-in-cache'
import { createInfinitePromise } from './infinite-promise'
import { NEXT_RSC_UNION_QUERY } from './app-router-headers'
Expand Down Expand Up @@ -89,24 +88,13 @@ export function urlToUrlWithoutFlightMarker(url: string): URL {
return urlWithoutFlightParameters
}

const HotReloader:
| typeof import('./react-dev-overlay/hot-reloader-client').default
| null =
process.env.NODE_ENV === 'production'
? null
: (require('./react-dev-overlay/hot-reloader-client')
.default as typeof import('./react-dev-overlay/hot-reloader-client').default)

type AppRouterProps = Omit<
Omit<InitialRouterStateParameters, 'isServer' | 'location'>,
'initialParallelRoutes'
> & {
buildId: string
initialHead: ReactNode
assetPrefix: string
// Top level boundaries props
notFound: React.ReactNode | undefined
asNotFound?: boolean
}

function isExternalURL(url: URL) {
Expand Down Expand Up @@ -224,8 +212,6 @@ function Router({
initialCanonicalUrl,
children,
assetPrefix,
notFound,
asNotFound,
}: AppRouterProps) {
const initialState = useMemo(
() =>
Expand Down Expand Up @@ -445,16 +431,26 @@ function Router({
return findHeadInCache(cache, tree[1])
}, [cache, tree])

const notFoundProps = { notFound, asNotFound }

const content = (
let content = (
<RedirectBoundary>
{head}
{cache.subTreeData}
<AppRouterAnnouncer tree={tree} />
</RedirectBoundary>
)

if (process.env.NODE_ENV !== 'production') {
if (typeof window !== 'undefined') {
const DevRootNotFoundBoundary: typeof import('./dev-root-not-found-boundary').DevRootNotFoundBoundary =
require('./dev-root-not-found-boundary').DevRootNotFoundBoundary
content = <DevRootNotFoundBoundary>{content}</DevRootNotFoundBoundary>
}
const HotReloader: typeof import('./react-dev-overlay/hot-reloader-client').default =
require('./react-dev-overlay/hot-reloader-client').default

content = <HotReloader assetPrefix={assetPrefix}>{content}</HotReloader>
}

return (
<>
<HistoryUpdater
Expand Down Expand Up @@ -484,16 +480,7 @@ function Router({
url: canonicalUrl,
}}
>
{HotReloader ? (
// HotReloader implements a separate NotFoundBoundary to maintain the HMR ping interval
<HotReloader assetPrefix={assetPrefix} {...notFoundProps}>
{content}
</HotReloader>
) : (
<NotFoundBoundary {...notFoundProps}>
{content}
</NotFoundBoundary>
)}
{content}
</LayoutRouterContext.Provider>
</AppRouterContext.Provider>
</GlobalLayoutRouterContext.Provider>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
'use client'

import React from 'react'
import { NotFoundBoundary } from './not-found-boundary'

export function bailOnNotFound() {
throw new Error('notFound() is not allowed to use in root layout')
}

function NotAllowedRootNotFoundError() {
bailOnNotFound()
return null
huozhi marked this conversation as resolved.
Show resolved Hide resolved
}

export function DevRootNotFoundBoundary({
children,
}: {
children: React.ReactNode
}) {
return (
<NotFoundBoundary notFound={<NotAllowedRootNotFoundError />}>
{children}
</NotFoundBoundary>
)
}
3 changes: 0 additions & 3 deletions packages/next/src/client/components/layout-router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,6 @@ export default function OuterLayoutRouter({
template,
notFound,
notFoundStyles,
asNotFound,
styles,
}: {
parallelRouterKey: string
Expand All @@ -506,7 +505,6 @@ export default function OuterLayoutRouter({
hasLoading: boolean
notFound: React.ReactNode | undefined
notFoundStyles: React.ReactNode | undefined
asNotFound?: boolean
styles?: React.ReactNode
}) {
const context = useContext(LayoutRouterContext)
Expand Down Expand Up @@ -574,7 +572,6 @@ export default function OuterLayoutRouter({
<NotFoundBoundary
notFound={notFound}
notFoundStyles={notFoundStyles}
asNotFound={asNotFound}
>
<RedirectBoundary>
<InnerLayoutRouter
Expand Down
3 changes: 3 additions & 0 deletions packages/next/src/client/components/not-found-boundary.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ class NotFoundErrorBoundary extends React.Component<
return (
<>
<meta name="robots" content="noindex" />
{process.env.NODE_ENV === 'development' && (
<meta name="next-error" content="not-found" />
)}
{this.props.notFoundStyles}
{this.props.notFound}
</>
Expand Down