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.

Sign up for GitHub

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

Improve create not found tree and remove asNotFound #68910

Merged
merged 3 commits into from
Aug 20, 2024
Merged

Improve create not found tree and remove asNotFound #68910

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
95ba6b9
improve create not found tree
huozhi Aug 14, 2024
4c763cd
Remove asNotFound from app-render
huozhi Aug 14, 2024
adb7026
fix not found
huozhi Aug 14, 2024
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
1 change: 0 additions & 1 deletion packages/next/src/server/app-render/action-handler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,7 +898,6 @@ export async function handleAction({
result: await generateFlight(req, ctx, {
skipFlight: false,
actionResult: promise,
asNotFound: true,
}),
}
}
Expand Down
44 changes: 24 additions & 20 deletions packages/next/src/server/app-render/app-render.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ import { createInitialRouterState } from '../../client/components/router-reducer
import { createMutableActionQueue } from '../../shared/lib/router/action-queue'
import { prerenderAsyncStorage } from './prerender-async-storage.external'
import { getRevalidateReason } from '../instrumentation/utils'
import { PAGE_SEGMENT_KEY } from '../../shared/lib/segment'

export type GetDynamicParamFromSegment = (
// [slug] / [[slug]] / [...slug]
Expand Down Expand Up @@ -228,7 +229,20 @@ function parseRequestHeaders(

function createNotFoundLoaderTree(loaderTree: LoaderTree): LoaderTree {
// Align the segment with parallel-route-default in next-app-loader
return ['', {}, loaderTree[2]]
const components = loaderTree[2]
return [
'',
{
children: [
PAGE_SEGMENT_KEY,
{},
{
page: components['not-found'],
},
],
},
components,
]
}

export type CreateSegmentPath = (child: FlightSegmentPath) => FlightSegmentPath
Expand Down Expand Up @@ -334,7 +348,6 @@ async function generateDynamicRSCPayload(
options?: {
actionResult: ActionResult
skipFlight: boolean
asNotFound?: boolean
}
): Promise<RSCPayload> {
// Flight data that is going to be passed to the browser.
Expand Down Expand Up @@ -382,7 +395,6 @@ async function generateDynamicRSCPayload(
injectedJS: new Set(),
injectedFontPreloadTags: new Set(),
rootLayoutIncluded: false,
asNotFound: ctx.isNotFoundPath || options?.asNotFound,
getMetadataReady,
preloadCallbacks,
})
Expand Down Expand Up @@ -434,7 +446,6 @@ async function generateDynamicFlightRenderResult(
options?: {
actionResult: ActionResult
skipFlight: boolean
asNotFound?: boolean
componentTree?: CacheNodeSeedData
preloadCallbacks?: PreloadCallbacks
}
Expand Down Expand Up @@ -475,7 +486,7 @@ async function generateDynamicFlightRenderResult(
async function getRSCPayload(
tree: LoaderTree,
ctx: AppRenderContext,
asNotFound: boolean
is404: boolean
) {
const injectedCSS = new Set<string>()
const injectedJS = new Set<string>()
Expand All @@ -502,7 +513,7 @@ async function getRSCPayload(

const [MetadataTree, getMetadataReady] = createMetadataComponents({
tree,
errorType: asNotFound ? 'not-found' : undefined,
errorType: is404 ? 'not-found' : undefined,
query,
metadataContext: createMetadataContext(url.pathname, ctx.renderOpts),
getDynamicParamFromSegment: getDynamicParamFromSegment,
Expand All @@ -522,7 +533,6 @@ async function getRSCPayload(
injectedJS,
injectedFontPreloadTags,
rootLayoutIncluded: false,
asNotFound: asNotFound,
getMetadataReady,
missingSlots,
preloadCallbacks,
Expand Down Expand Up @@ -756,6 +766,9 @@ async function renderToHTMLOrFlightImpl(
requestEndedState: { ended?: boolean }
) {
const isNotFoundPath = pagePath === '/404'
if (isNotFoundPath) {
res.statusCode = 404
}

// A unique request timestamp used by development to ensure that it's
// consistent and won't change during this request. This is important to
Expand Down Expand Up @@ -945,14 +958,12 @@ async function renderToHTMLOrFlightImpl(
prerenderToStream
)

const asNotFound = isNotFoundPath
let response = await prerenderToStreamWithTracing(
req,
res,
ctx,
metadata,
staticGenerationStore,
asNotFound,
loaderTree
)

Expand Down Expand Up @@ -1057,12 +1068,11 @@ async function renderToHTMLOrFlightImpl(
if (actionRequestResult) {
if (actionRequestResult.type === 'not-found') {
const notFoundLoaderTree = createNotFoundLoaderTree(loaderTree)
const asNotFound = true
res.statusCode = 404
const stream = await renderToStreamWithTracing(
req,
res,
ctx,
asNotFound,
notFoundLoaderTree,
formState
)
Expand All @@ -1083,12 +1093,10 @@ async function renderToHTMLOrFlightImpl(
metadata,
}

const asNotFound = isNotFoundPath
const stream = await renderToStreamWithTracing(
req,
res,
ctx,
asNotFound,
loaderTree,
formState
)
Expand Down Expand Up @@ -1185,8 +1193,6 @@ async function renderToStream(
req: BaseNextRequest,
res: BaseNextResponse,
ctx: AppRenderContext,

asNotFound: boolean,
tree: LoaderTree,
formState: any
): Promise<ReadableStream<Uint8Array>> {
Expand Down Expand Up @@ -1273,7 +1279,7 @@ async function renderToStream(

try {
// This is a dynamic render. We don't do dynamic tracking because we're not prerendering
const RSCPayload = await getRSCPayload(tree, ctx, asNotFound)
const RSCPayload = await getRSCPayload(tree, ctx, res.statusCode === 404)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we can just infer it is a 404 from the tree itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

might need more checks since a non-existed (/404) page might have slight different tree from the not-found tree we created when it's not-found path. I'll keep it in mind to see if we can optimize it later

const reactServerStream = ComponentMod.renderToReadableStream(
RSCPayload,
clientReferenceManifest.clientModules,
Expand Down Expand Up @@ -1594,8 +1600,6 @@ async function prerenderToStream(
ctx: AppRenderContext,
metadata: AppPageRenderResultMetadata,
staticGenerationStore: StaticGenerationStore,

asNotFound: boolean,
tree: LoaderTree
): Promise<PrenderToStringResult> {
// When prerendering formState is always null. We still include it
Expand Down Expand Up @@ -1700,7 +1704,7 @@ async function prerenderToStream(
const reactServerPrerenderStore = {
dynamicTracking,
}
const RSCPayload = await getRSCPayload(tree, ctx, asNotFound)
const RSCPayload = await getRSCPayload(tree, ctx, res.statusCode === 404)
const reactServerStream: ReadableStream<Uint8Array> =
prerenderAsyncStorage.run(
reactServerPrerenderStore,
Expand Down Expand Up @@ -1864,7 +1868,7 @@ async function prerenderToStream(
} else {
// This is a regular static generation. We don't do dynamic tracking because we rely on
// the old-school dynamic error handling to bail out of static generation
const RSCPayload = await getRSCPayload(tree, ctx, asNotFound)
const RSCPayload = await getRSCPayload(tree, ctx, res.statusCode === 404)
const reactServerStream = ComponentMod.renderToReadableStream(
RSCPayload,
clientReferenceManifest.clientModules,
Expand Down
25 changes: 0 additions & 25 deletions packages/next/src/server/app-render/create-component-tree.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ export function createComponentTree(props: {
injectedCSS: Set<string>
injectedJS: Set<string>
injectedFontPreloadTags: Set<string>
asNotFound?: boolean
getMetadataReady: () => Promise<void>
ctx: AppRenderContext
missingSlots?: Set<string>
Expand Down Expand Up @@ -65,7 +64,6 @@ async function createComponentTreeInternal({
injectedCSS,
injectedJS,
injectedFontPreloadTags,
asNotFound,
getMetadataReady,
ctx,
missingSlots,
Expand All @@ -79,7 +77,6 @@ async function createComponentTreeInternal({
injectedCSS: Set<string>
injectedJS: Set<string>
injectedFontPreloadTags: Set<string>
asNotFound?: boolean
getMetadataReady: () => Promise<void>
ctx: AppRenderContext
missingSlots?: Set<string>
Expand Down Expand Up @@ -437,7 +434,6 @@ async function createComponentTreeInternal({
injectedCSS: injectedCSSWithCurrentLayout,
injectedJS: injectedJSWithCurrentLayout,
injectedFontPreloadTags: injectedFontPreloadTagsWithCurrentLayout,
asNotFound,
// getMetadataReady is used to conditionally throw. In the case of parallel routes we will have more than one page
// but we only want to throw on the first one.
getMetadataReady: isChildrenRouteKey
Expand Down Expand Up @@ -545,27 +541,6 @@ async function createComponentTreeInternal({
// We avoid cloning this object because it gets consumed here exclusively.
const props: { [prop: string]: any } = parallelRouteProps

// If it's a not found route, and we don't have any matched parallel
// routes, we try to render the not found component if it exists.
if (
NotFound &&
asNotFound &&
// In development, it could hit the parallel-route-default not found, so we only need to check the segment.
// Or if there's no parallel routes means it reaches the end.
!parallelRouteMap.length
) {
props.children = (
<>
<meta name="robots" content="noindex" />
lubieowoce marked this conversation as resolved.
Show resolved Hide resolved
{process.env.NODE_ENV === 'development' && (
<meta name="next-error" content="not-found" />
)}
{notFoundStyles}
<NotFound />
</>
)
}

// Assign params to props
if (
process.env.NODE_ENV === 'development' &&
Expand Down
4 changes: 0 additions & 4 deletions packages/next/src/server/app-render/walk-tree-with-flight-router-state.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ export async function walkTreeWithFlightRouterState({
injectedJS,
injectedFontPreloadTags,
rootLayoutIncluded,
asNotFound,
getMetadataReady,
ctx,
preloadCallbacks,
Expand All @@ -53,7 +52,6 @@ export async function walkTreeWithFlightRouterState({
injectedJS: Set<string>
injectedFontPreloadTags: Set<string>
rootLayoutIncluded: boolean
asNotFound?: boolean
getMetadataReady: () => Promise<void>
ctx: AppRenderContext
preloadCallbacks: PreloadCallbacks
Expand Down Expand Up @@ -153,7 +151,6 @@ export async function walkTreeWithFlightRouterState({
injectedFontPreloadTags,
// This is intentionally not "rootLayoutIncludedAtThisLevelOrAbove" as createComponentTree starts at the current level and does a check for "rootLayoutAtThisLevel" too.
rootLayoutIncluded,
asNotFound,
getMetadataReady,
preloadCallbacks,
}
Expand Down Expand Up @@ -214,7 +211,6 @@ export async function walkTreeWithFlightRouterState({
injectedJS: injectedJSWithCurrentLayout,
injectedFontPreloadTags: injectedFontPreloadTagsWithCurrentLayout,
rootLayoutIncluded: rootLayoutIncludedAtThisLevelOrAbove,
asNotFound,
getMetadataReady,
preloadCallbacks,
})
Expand Down
Loading